Subject: Re: [PATCH] drm: fourcc byteorder: brings header file comments in
 line with reality.
To: =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        Gerd Hoffmann <kraxel@redhat.com>
References: <20170421075825.6307-1-kraxel@redhat.com>
 <20170421092530.GE30290@intel.com> <1492768218.25675.33.camel@redhat.com>
 <20170421110804.GH30290@intel.com>
 <f1b0ae60-b20e-ad52-7fbf-32f2c045f5fc@amd.com>
 <1492780323.25675.45.camel@redhat.com>
 <041de509-f56f-f460-1961-810bc5a3c18e@vodafone.de>
 <fab1354d-bb5d-b196-a698-f465906ffc68@amd.com>
CC: <dri-devel@lists.freedesktop.org>,
        Daniel Vetter <daniel.vetter@intel.com>,
        =?UTF-8?Q?Michel_D=c3=a4nzer?= <michel@daenzer.net>,
        open list <linux-kernel@vger.kernel.org>,
        <amd-gfx@lists.freedesktop.org>
From: Harry Wentland <harry.wentland@amd.com>
Message-ID: <f64a3742-de57-d62a-d7da-bcaa3bef0dd9@amd.com>
Date: Fri, 21 Apr 2017 11:21:13 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <fab1354d-bb5d-b196-a698-f465906ffc68@amd.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Apr 2017 15:21:15.9044 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR1201MB0009
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 4329
Lines: 115

Thanks, Christian for adding me.

On 2017-04-21 09:27 AM, Christian König wrote:
> Adding Harry to this mail thread as well, cause is one of the people
> really affected by this.
>
> Christian.
>
> Am 21.04.2017 um 15:21 schrieb Christian König:
>> Am 21.04.2017 um 15:12 schrieb Gerd Hoffmann:
>>>    Hi,
>>>
>>>>> "native" to me feels more like "native to the GPU" since these things
>>>>> really are tied to the GPU not the CPU. That's also why I went with
>>>>> the
>>>>> explicit endianness originally so that the driver could properly
>>>>> declare
>>>>> what the GPU supports.
>>>> And to be honest I would really prefer to stick with that approach for
>>>> exactly that reason.
>>>>

I strongly agree with Christian and Ville on this. I understand fourcc 
endianness as GPU endianness. Usermode needs to be clear on the 
framebuffer format, whether it's GPU or CPU rendered, so kernel should 
define this clearly.

In practice it probably doesn't currently make much difference for AMD 
GPUs. I've never heard of anyone using them on big-endian systems.

Harry


>>>> The proposed change would require that drivers have different code path
>>>> for different CPU byte order. Those code path tend to be not tested
>>>> very
>>>> well and are additional complexity we probably don't want inside the
>>>> driver.
>>> We can add fixed-endian #defines without too much effort, at least for
>>> the 8 bits per color formats.  In qemu we have the same problem, only
>>> with pixman.  Those formats are native endian too, but often we have to
>>> handle a fixed format, so we did this:
>>>
>>> /*
>>>   * pixman image formats are defined to be native endian,
>>>   * that means host byte order on qemu.  So we go define
>>>   * fixed formats here for cases where it is needed, like
>>>   * feeding libjpeg / libpng and writing screenshots.
>>>   */
>>>
>>> #ifdef HOST_WORDS_BIGENDIAN
>>> # define PIXMAN_BE_r8g8b8     PIXMAN_r8g8b8
>>> # define PIXMAN_BE_x8r8g8b8   PIXMAN_x8r8g8b8
>>> # define PIXMAN_BE_a8r8g8b8   PIXMAN_a8r8g8b8
>>> # define PIXMAN_BE_b8g8r8x8   PIXMAN_b8g8r8x8
>>> # define PIXMAN_BE_b8g8r8a8   PIXMAN_b8g8r8a8
>>> # define PIXMAN_BE_r8g8b8x8   PIXMAN_r8g8b8x8
>>> # define PIXMAN_BE_r8g8b8a8   PIXMAN_r8g8b8a8
>>> # define PIXMAN_BE_x8b8g8r8   PIXMAN_x8b8g8r8
>>> # define PIXMAN_BE_a8b8g8r8   PIXMAN_a8b8g8r8
>>> # define PIXMAN_LE_x8r8g8b8   PIXMAN_b8g8r8x8
>>> #else
>>> # define PIXMAN_BE_r8g8b8     PIXMAN_b8g8r8
>>> # define PIXMAN_BE_x8r8g8b8   PIXMAN_b8g8r8x8
>>> # define PIXMAN_BE_a8r8g8b8   PIXMAN_b8g8r8a8
>>> # define PIXMAN_BE_b8g8r8x8   PIXMAN_x8r8g8b8
>>> # define PIXMAN_BE_b8g8r8a8   PIXMAN_a8r8g8b8
>>> # define PIXMAN_BE_r8g8b8x8   PIXMAN_x8b8g8r8
>>> # define PIXMAN_BE_r8g8b8a8   PIXMAN_a8b8g8r8
>>> # define PIXMAN_BE_x8b8g8r8   PIXMAN_r8g8b8x8
>>> # define PIXMAN_BE_a8b8g8r8   PIXMAN_r8g8b8a8
>>> # define PIXMAN_LE_x8r8g8b8   PIXMAN_x8r8g8b8
>>> #endif
>>
>> Exactly what Mesa did as well.
>>
>>>> My personal opinion is that formats in drm_fourcc.h should be
>>>> independent of the CPU byte order and the function
>>>> drm_mode_legacy_fb_format() and drivers depending on that incorrect
>>>> assumption be fixed instead.
>>> The problem is this isn't a kernel-internal thing any more. With the
>>> addition of the ADDFB2 ioctl the fourcc codes became part of the
>>> kernel/userspace abi ...
>>
>> I know and that's exactly the reason I'm going to object those changes.
>>
>> The kernel/userspace abi is fixed and changing it like this could
>> potentially break drivers I'm the co-maintainer of. So that whole
>> approach is a clear NAK from my side.
>>
>> If you find a driver or userspace which doesn't use the formats as
>> defined in the comments of drm_fourcc.h the fix the driver instead of
>> trying to adjust the common header to broken behavior. Cause the later
>> will clearly cause problems with drivers who correctly implemented the
>> interface.
>>
>> Regards,
>> Christian.
>>
>>>
>>> cheers,
>>>    Gerd
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>