On Fri, Feb 27, 2015 at 09:45:27AM +0000, Tvrtko Ursulin wrote:
>
> On 02/26/2015 04:44 PM, Daniel Vetter wrote:
> >On Wed, Feb 25, 2015 at 04:47:24PM +0000, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >>
> >>By this patch all underlying bits have been implemented and this
> >>patch actually enables the feature.
> >>
> >>v2: Validate passed in fb modifiers to reject garbage. (Daniel Vetter)
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >>Reviewed-by: Damien Lespiau <damien.lespiau@xxxxxxxxx> (v1)
> >>---
> >> drivers/gpu/drm/i915/intel_display.c | 26 +++++++++++++++++++++-----
> >> 1 file changed, 21 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>index 1d50934..3232ddd 100644
> >>--- a/drivers/gpu/drm/i915/intel_display.c
> >>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>@@ -12783,6 +12783,19 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >> WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >>
> >> if (mode_cmd->flags & DRM_MODE_FB_MODIFIERS) {
> >>+ /* Passed in modifier sanity checking. */
> >>+ switch (mode_cmd->modifier[0]) {
> >>+ case DRM_FORMAT_MOD_NONE:
> >>+ case I915_FORMAT_MOD_X_TILED:
> >>+ case I915_FORMAT_MOD_Y_TILED:
> >>+ case I915_FORMAT_MOD_Yf_TILED:
> >>+ break;
> >>+ default:
> >>+ DRM_ERROR("Unsupported fb modifier 0x%llx!\n",
> >>+ mode_cmd->modifier[0]);
> >>+ return -EINVAL;
> >>+ }
> >>+
> >> /* Enforce that fb modifier and tiling mode match, but only for
> >> * X-tiled. This is needed for FBC. */
> >> if (!!(obj->tiling_mode == I915_TILING_X) !=
> >>@@ -12790,6 +12803,14 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >> DRM_DEBUG("tiling_mode doesn't match fb modifier\n");
> >> return -EINVAL;
> >> }
> >>+
> >>+ if (INTEL_INFO(dev)->gen < 9 &&
> >>+ (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> >>+ mode_cmd->modifier[0] == I915_FORMAT_MOD_Yf_TILED)) {
> >>+ DRM_DEBUG("Unsupported tiling 0x%llx!\n",
> >>+ mode_cmd->modifier[0]);
> >>+ return -EINVAL;
> >>+ }
> >> } else {
> >> if (obj->tiling_mode == I915_TILING_X)
> >> mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED;
> >>@@ -12799,11 +12820,6 @@ static int intel_framebuffer_init(struct drm_device *dev,
> >> }
> >> }
> >>
> >>- if (mode_cmd->modifier[0] == I915_FORMAT_MOD_Y_TILED) {
> >>- DRM_DEBUG("hardware does not support tiling Y\n");
> >>- return -EINVAL;
> >>- }
> >
> >My idea was actually to put the switch here (reduces one indent level) and
> >put the per-gen stuff into the switch statement too. I.e.
> >
> > /* Passed in modifier sanity checking. */
> > switch (mode_cmd->modifier[0]) {
> > case I915_FORMAT_MOD_Y_TILED:
> > case I915_FORMAT_MOD_Yf_TILED:
> > if (INTEL_INFO(dev)->gen < 9) {
> > DRM_DEBUG("Unsupported tiling 0x%llx!\n",
> > mode_cmd->modifier[0]);
> > return -EINVAL;
> >
> > }
> > case DRM_FORMAT_MOD_NONE:
> > case I915_FORMAT_MOD_X_TILED:
> > break;
> > default:
> > DRM_ERROR("Unsupported fb modifier 0x%llx!\n",
> > mode_cmd->modifier[0]);
> > return -EINVAL;
> > }
> >
> >That gives us a natural place for extensions later on if we need to add
> >more special cases.
>
> Yes I agree this patch being only on v2 was way disproportionate compared to
> some others from this series. ;)
Well I've obviously failed to explain properly in my review to v1 to avoid
a v3 ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
