On 02/02/2015 05:15 PM, Daniel Vetter wrote:
On Mon, Feb 02, 2015 at 10:36:30AM +0000, Tvrtko Ursulin wrote:
On 02/02/2015 09:54 AM, Daniel Vetter wrote:
On Fri, Jan 30, 2015 at 05:36:57PM +0000, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Use the fb modifier if it was specified over object tiling mode.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++++++++++-------
1 file changed, 33 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e22afbe..ca69da0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12671,6 +12671,20 @@ static const struct drm_framebuffer_funcs intel_fb_funcs = {
.create_handle = intel_user_framebuffer_create_handle,
};
+static unsigned int
+intel_fb_modifier_to_tiling(u64 modifier)
+{
+ switch (modifier) {
+ case I915_FORMAT_MOD_X_TILED:
+ return I915_TILING_X;
+ default:
+ case I915_FORMAT_MOD_NONE:
+ break;
+ }
+
+ return I915_TILING_NONE;
+}
+
static int intel_framebuffer_init(struct drm_device *dev,
struct intel_framebuffer *intel_fb,
struct drm_mode_fb_cmd2 *mode_cmd,
@@ -12678,11 +12692,23 @@ static int intel_framebuffer_init(struct drm_device *dev,
{
int aligned_height;
int pitch_limit;
+ unsigned int tiling_mode = obj->tiling_mode;
int ret;
WARN_ON(!mutex_is_locked(&dev->struct_mutex));
- if (obj->tiling_mode == I915_TILING_Y) {
+ if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
+ tiling_mode =
+ intel_fb_modifier_to_tiling(mode_cmd->modifier[0]);
+ if (tiling_mode != obj->tiling_mode &&
+ obj->tiling_mode != I915_TILING_NONE) {
+ DRM_ERROR("Tiling modifier mismatch %u vs obj %u!\n",
+ tiling_mode, obj->tiling_mode);
+ return -EINVAL;
+ }
+ }
Ah, here comes the magic. I think this might be simpler if we just use
->modifier (and fix it up if FB_MODIFIERS isn't set).
Btw another reason for this split is that this way we have a clear
separation between the tiling modes supported generally (as fb modifiers)
and the tiling modes supported by fences. It might therefore make sense to
rename obj->tiling_mode with a cocci patch to obj->fencing_mode or
->fence_tiling_mode). To make it really clear that it's just about the
global gtt fences and nothing more.
I don't really like using ->modifier directly in tiling patch since it is an
bag of unrelated stuff, not only a superset. Unrelated especially, but not
only, from the point of view of call sites / users.
Therefore I see some design elegance in extracting the tiling, or any other
logical group of modifiers before hand.
At the very least would call something like intel_fb_modifier_to_tiling(),
but, it is very ugly to have a dynamic cost at every call site. Which is
another reason why I preferred to extract the data before hand.
The reason is that the current tiling_mode enum is userspace ABI, and
it's just for how to fence global gtt mappings. That's the point of
splitting the fb modifiers out like in this rfc.
So if you add your fancy new tiling mode you can't do that, since you
can't extend the tiling_mode enum. Adding another enum also seems a bit
Why not? It is not changing the ABI since obj->tiling_mode stays exactly
the same as it is today.
Do you worry about leaking new data out in i915_drm.h, under the
I915_TILING_* #defines? I don't see that we have to change that at all.
too much when we already have fb_modifiers.
And if fb_modifiers get too complicated we can add helper functions which
normalize stuff, e.g. extract just the base tiling mode and remove other
things (like compression mode or whatever it's going to be).
So you are strongly for "looking into a bag of stuff" to see if anything
interesting is there on every call site?
Helper functions in my view only marginally help there - they make the
code neater but design is conceptually still untidy. And you add
pointless processing on every call site.
I just don't see what is the problem with extracting the interesting
data "from the bag" at fb init time. If you tried to make some
synchronization argument in the other reply I don't get it.
fb->modifier[0] should be, in my opinion, viewed as immutable. And it
lives at the base class level while in intel_frambuffer sub-class it
should be just fine to "parse" that into directly usable data stored at
the sub-class level.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
