Subject: Re: [PATCH] drm: fourcc byteorder: brings header file comments in
 line with reality.
To: Gerd Hoffmann <kraxel@redhat.com>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.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>
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>,
        "Wentland, Harry" <Harry.Wentland@amd.com>
From: =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
Message-ID: <fab1354d-bb5d-b196-a698-f465906ffc68@amd.com>
Date: Fri, 21 Apr 2017 15:27:47 +0200
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: <041de509-f56f-f460-1961-810bc5a3c18e@vodafone.de>
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 13:27:59.9833 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR12MB1309
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3773
Lines: 99

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.
>>>
>>> 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