[Mesa-dev] [PATCH v2 1/2] i965: Use helper function for modifier -> tiling

[Emil Velikov]

On 4 May 2017 at 14:43, Daniel Stone <daniel at fooishbar.org> wrote:
> Hi Emil,
>
> On 4 May 2017 at 13:27, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>> @@ -581,21 +600,17 @@ intel_create_image_common(__DRIscreen *dri_screen,
>>>     assert(!(use && count));
>>>
>>>     uint64_t modifier = select_best_modifier(&screen->devinfo, modifiers, count);
>>> -   switch (modifier) {
>>> -   case I915_FORMAT_MOD_X_TILED:
>>> -      assert(tiling == I915_TILING_X);
>>> -      break;
>>> -   case DRM_FORMAT_MOD_LINEAR:
>>> -      tiling = I915_TILING_NONE;
>>> -      break;
>>> -   case I915_FORMAT_MOD_Y_TILED:
>>> -      tiling = I915_TILING_Y;
>>> -      break;
>>> -   case DRM_FORMAT_MOD_INVALID:
>>> +   if (modifier == DRM_FORMAT_MOD_INVALID) {
>>> +      /* User requested specific modifiers, none of which work */
>>>        if (modifiers)
>>>           return NULL;
>>> -   default:
>>> -         break;
>> Originally, here we'll use I915_TILING_X...
>>
>>> +
>>> +      /* Historically, X-tiled was the default, and so lack of modifier means
>>> +       * X-tiled.
>>> +       */
>>> +      tiling = I915_TILING_X;
>>> +   } else {
>>> +      tiling = modifier_to_tiling(modifier);
>> ... while now we'll get I915_TILING_NONE.
>
> I don't think so ... ?
>
> If we don't find a modifier to use (LINEAR/X_TILED/Y_TILED) from
> select_best_modifier(), then we return INVALID, which hits the first
> branch above: failure if the user provided a set of modifiers we don't
> support, or TILING_X if the user didn't provide a set of modifiers. If
> we hit this branch here, then the user has explicitly requested one of
> the three modifiers we support.
>
Thanks for the correction Dan. Having a look in select_best_modifier()
would have helped ;-)

> IOW, there is no change as far as I can see, but perhaps for the
> meantime, we could use an unreachable() at the bottom of
> modifier_to_tiling(). Would that help?
>
Yes the default statement and return I915_TILING_NONE in
modifier_to_tiling() seem unreachable.

unreachable or assert - I am leaning slightly towards the latter since
it will catch fallouts in the future.
I'll leave the decision to you and the Intel devs.

In either case:
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

Emil