Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

  Hi,

> > So just not using the swapping indeed looks like the only sensible
> > option.  Which in turn implies there is no BGRA support for dumb
> > bos.  Hmm, I can see the problem.  Userspace expectation appears to be
> > that ADDFB configures a native endian framebuffer, which the driver
> > simply can't do on bigendian.
> 
> ... with pre-R600 GPUs.

Sure.

> > So, what can/should the driver do here?  Throw errors for ADDFB and
> > force userspace to use ADDFB2?  From a design point of view the best
> > option, but in the other hand I suspect that could break the xorg radeon
> > driver ...
> 
> Yes, it would.

> One thing we could do is provide a way for userspace to query the
> effective format(s) as seen by the GPU and/or CPU.

We already have almost no testing on bigendian.  I doubt adding generic
interfaces specifically to handle this case is going to work because
most userspace will simply not implement that correctly (or at all).

Having support for this in the radeon ioctls might work, because only
radeon kernel + xorg driver have to get things right then.  But I
suspect we already have that.  You've mentioned elsewhere in the thread
that the xorg driver doesn't turn on byteswapping, so the ability to
configure that seems to be somewhere in the API ...

> It might also make sense for the radeon driver to set the
> RADEON_TILING_SWAP_{16,32}BIT flags for dumb BOs.

That could work.  But I guess someone with test hardware needs to try,
to make sure we don't miss corner cases here.

cheers,
  Gerd

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Michel Dänzer]

On 27/04/17 03:45 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>>> That is done using the RADEON_TILING_SWAP_{16,32}BIT flag mentioned in
>>> another thread?
>>
>> Right.
>>
>>
>>> What about dumb bos?  You've mentioned the swap flag isn't used for
>>> those.  Which implies they are in little endian byte order (both gpu and
>>> cpu view).
>>
>> Right, AFAICT from looking at the code.
> 
> Ok.
> 
> And I also don't see an easy way to make them big endian (cpu view)
> using swapping with the existing drm interfaces, given we apply a format
> when we put the bo into use as framebuffer, not when creating it.  So
> userspace can: (1) create dumb bo, (2) map bo, (3) write something bo,
> (4) create fb + attach to crtc.  And at (3) we don't know the format
> yet, so we can't configure swapping accordingly.
> 
> So just not using the swapping indeed looks like the only sensible
> option.  Which in turn implies there is no BGRA support for dumb
> bos.  Hmm, I can see the problem.  Userspace expectation appears to be
> that ADDFB configures a native endian framebuffer, which the driver
> simply can't do on bigendian.

... with pre-R600 GPUs.


> So, what can/should the driver do here?  Throw errors for ADDFB and
> force userspace to use ADDFB2?  From a design point of view the best
> option, but in the other hand I suspect that could break the xorg radeon
> driver ...

Yes, it would.

One thing we could do is provide a way for userspace to query the
effective format(s) as seen by the GPU and/or CPU.

It might also make sense for the radeon driver to set the
RADEON_TILING_SWAP_{16,32}BIT flags for dumb BOs. I wonder about the
status of apps using dumb BOs directly wrt this discussion.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

  Hi,

> > That is done using the RADEON_TILING_SWAP_{16,32}BIT flag mentioned in
> > another thread?
> 
> Right.
> 
> 
> > What about dumb bos?  You've mentioned the swap flag isn't used for
> > those.  Which implies they are in little endian byte order (both gpu and
> > cpu view).
> 
> Right, AFAICT from looking at the code.

Ok.

And I also don't see an easy way to make them big endian (cpu view)
using swapping with the existing drm interfaces, given we apply a format
when we put the bo into use as framebuffer, not when creating it.  So
userspace can: (1) create dumb bo, (2) map bo, (3) write something bo,
(4) create fb + attach to crtc.  And at (3) we don't know the format
yet, so we can't configure swapping accordingly.

So just not using the swapping indeed looks like the only sensible
option.  Which in turn implies there is no BGRA support for dumb
bos.  Hmm, I can see the problem.  Userspace expectation appears to be
that ADDFB configures a native endian framebuffer, which the driver
simply can't do on bigendian.

So, what can/should the driver do here?  Throw errors for ADDFB and
force userspace to use ADDFB2?  From a design point of view the best
option, but in the other hand I suspect that could break the xorg radeon
driver ...

cheers,
  Gerd

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Michel Dänzer]

On 26/04/17 09:11 PM, Gerd Hoffmann wrote:
>   Hi,
> 
 Just to reiterate, this won't work for the radeon driver, which programs
 the GPU to use (effectively, per the current definition that these are
 little endian GPU formats) DRM_FORMAT_XRGB with pre-R600 and
 DRM_FORMAT_BGRX with >= R600.
>>>
>>> Hmm, ok, how does bigendian fbdev emulation work on pre-R600 then?
>>
>> Using a GPU byte swapping mechanism which only affects CPU access to
>> video RAM.
> 
> That is done using the RADEON_TILING_SWAP_{16,32}BIT flag mentioned in
> another thread?

Right.


> What about dumb bos?  You've mentioned the swap flag isn't used for
> those.  Which implies they are in little endian byte order (both gpu and
> cpu view).

Right, AFAICT from looking at the code.


> Does the modesetting driver work correctly on that hardware?

Not sure.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Ville Syrjälä]

On Wed, Apr 26, 2017 at 11:00:09AM +0900, Michel Dänzer wrote:
> On 25/04/17 06:52 PM, Ville Syrjälä wrote:
> > On Tue, Apr 25, 2017 at 12:18:52PM +0900, Michel Dänzer wrote:
> >> On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
> >>> +#ifdef __BIG_ENDIAN
> >>> + switch (bpp) {
> >>> + case 8:
> >>> + fmt = DRM_FORMAT_C8;
> >>> + break;
> >>> + case 24:
> >>> + fmt = DRM_FORMAT_BGR888;
> >>> + break;
> >>
> >> BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.
> > 
> > To 8bpp no, but it can easily apply to 24bpp.
> 
> Any byte swapping rips apart the bytes of a 24bpp pixel, so those
> formats only make sense as straight array formats.

In my book little endian just means "lsb is stored in the lowest
memory address". The fact that your CPU/GPU can't do 3 byte swaps
is not relevant for that definition IMO.

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

>   uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
>   {
>   uint32_t fmt;
>   #ifdef __BIG_ENDIAN
>   enum { LITTLE_ENDIAN = 0 };
>   #else
>   enum { LITTLE_ENDIAN = 1 };
>   #endif
>   /* ... */
> 
> (using an enum for compile-time constness)
> 
> and then
>   fmt = DRM_FORMAT_ARGB;
> becomes
>   fmt = LITTLE_ENDIAN ? DRM_FORMAT_ARGB : DRM_FORMAT_BGRA;
> 
> Might be easier to read than duplicating the whole switch?

Well, there are more differences, like rgb565 and xrgb2101010 not being
supported for bigendian, so it isn't *that* simple.

cheers,
  Gerd

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Eric Engestrom]

On Wednesday, 2017-04-26 07:53:10 +0200, Gerd Hoffmann wrote:
> On Di, 2017-04-25 at 12:18 +0900, Michel Dänzer wrote:
> > On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
> > > Return correct fourcc codes on bigendian.  Drivers must be adapted to
> > > this change.
> > > 
> > > Signed-off-by: Gerd Hoffmann 
> > 
> > Just to reiterate, this won't work for the radeon driver, which programs
> > the GPU to use (effectively, per the current definition that these are
> > little endian GPU formats) DRM_FORMAT_XRGB with pre-R600 and
> > DRM_FORMAT_BGRX with >= R600.
> 
> Hmm, ok, how does bigendian fbdev emulation work on pre-R600 then?
> 
> > > +#ifdef __BIG_ENDIAN
> > > + switch (bpp) {
> > > + case 8:
> > > + fmt = DRM_FORMAT_C8;
> > > + break;
> > > + case 24:
> > > + fmt = DRM_FORMAT_BGR888;
> > > + break;
> > 
> > BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.
> 
> I could move the 8 bpp case out of the #ifdef somehow, but code
> readability will suffer then I think ...

How about something like this?

uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
{
uint32_t fmt;
#ifdef __BIG_ENDIAN
enum { LITTLE_ENDIAN = 0 };
#else
enum { LITTLE_ENDIAN = 1 };
#endif
/* ... */

(using an enum for compile-time constness)

and then
fmt = DRM_FORMAT_ARGB;
becomes
fmt = LITTLE_ENDIAN ? DRM_FORMAT_ARGB : DRM_FORMAT_BGRA;

Might be easier to read than duplicating the whole switch?

> 
> For 24 we have different byte orderings, but yes, you can't switch from
> one to the other with byteswapping.  Probably one of the reasons why
> this format is pretty much out of fashion these days ...
> 
> cheers,
>   Gerd
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

  Hi,

> >> Just to reiterate, this won't work for the radeon driver, which programs
> >> the GPU to use (effectively, per the current definition that these are
> >> little endian GPU formats) DRM_FORMAT_XRGB with pre-R600 and
> >> DRM_FORMAT_BGRX with >= R600.
> > 
> > Hmm, ok, how does bigendian fbdev emulation work on pre-R600 then?
> 
> Using a GPU byte swapping mechanism which only affects CPU access to
> video RAM.

That is done using the RADEON_TILING_SWAP_{16,32}BIT flag mentioned in
another thread?

Ok, so the cpu view to fbdev is DRM_FORMAT_BGRX in all cases.

What about dumb bos?  You've mentioned the swap flag isn't used for
those.  Which implies they are in little endian byte order (both gpu and
cpu view).  Does the modesetting driver work correctly on that hardware?

cheers,
  Gerd

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Michel Dänzer]

On 26/04/17 02:53 PM, Gerd Hoffmann wrote:
> On Di, 2017-04-25 at 12:18 +0900, Michel Dänzer wrote:
>> On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
>>> Return correct fourcc codes on bigendian.  Drivers must be adapted to
>>> this change.
>>>
>>> Signed-off-by: Gerd Hoffmann 
>>
>> Just to reiterate, this won't work for the radeon driver, which programs
>> the GPU to use (effectively, per the current definition that these are
>> little endian GPU formats) DRM_FORMAT_XRGB with pre-R600 and
>> DRM_FORMAT_BGRX with >= R600.
> 
> Hmm, ok, how does bigendian fbdev emulation work on pre-R600 then?

Using a GPU byte swapping mechanism which only affects CPU access to
video RAM.


>>> +#ifdef __BIG_ENDIAN
>>> +   switch (bpp) {
>>> +   case 8:
>>> +   fmt = DRM_FORMAT_C8;
>>> +   break;
>>> +   case 24:
>>> +   fmt = DRM_FORMAT_BGR888;
>>> +   break;
>>
>> BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.
> 
> I could move the 8 bpp case out of the #ifdef somehow, but code
> readability will suffer then I think ...

How so?

At least it would make clearer which formats are affected by endianness
and which aren't.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Gerd Hoffmann]

On Di, 2017-04-25 at 12:18 +0900, Michel Dänzer wrote:
> On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
> > Return correct fourcc codes on bigendian.  Drivers must be adapted to
> > this change.
> > 
> > Signed-off-by: Gerd Hoffmann 
> 
> Just to reiterate, this won't work for the radeon driver, which programs
> the GPU to use (effectively, per the current definition that these are
> little endian GPU formats) DRM_FORMAT_XRGB with pre-R600 and
> DRM_FORMAT_BGRX with >= R600.

Hmm, ok, how does bigendian fbdev emulation work on pre-R600 then?

> > +#ifdef __BIG_ENDIAN
> > +   switch (bpp) {
> > +   case 8:
> > +   fmt = DRM_FORMAT_C8;
> > +   break;
> > +   case 24:
> > +   fmt = DRM_FORMAT_BGR888;
> > +   break;
> 
> BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.

I could move the 8 bpp case out of the #ifdef somehow, but code
readability will suffer then I think ...

For 24 we have different byte orderings, but yes, you can't switch from
one to the other with byteswapping.  Probably one of the reasons why
this format is pretty much out of fashion these days ...

cheers,
  Gerd

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Michel Dänzer]

On 25/04/17 06:52 PM, Ville Syrjälä wrote:
> On Tue, Apr 25, 2017 at 12:18:52PM +0900, Michel Dänzer wrote:
>> On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
>>> +#ifdef __BIG_ENDIAN
>>> +   switch (bpp) {
>>> +   case 8:
>>> +   fmt = DRM_FORMAT_C8;
>>> +   break;
>>> +   case 24:
>>> +   fmt = DRM_FORMAT_BGR888;
>>> +   break;
>>
>> BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.
> 
> To 8bpp no, but it can easily apply to 24bpp.

Any byte swapping rips apart the bytes of a 24bpp pixel, so those
formats only make sense as straight array formats.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Ville Syrjälä]

On Tue, Apr 25, 2017 at 12:18:52PM +0900, Michel Dänzer wrote:
> On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
> > Return correct fourcc codes on bigendian.  Drivers must be adapted to
> > this change.
> > 
> > Signed-off-by: Gerd Hoffmann 
> 
> Just to reiterate, this won't work for the radeon driver, which programs
> the GPU to use (effectively, per the current definition that these are
> little endian GPU formats) DRM_FORMAT_XRGB with pre-R600 and
> DRM_FORMAT_BGRX with >= R600.
> 
> 
> > +#ifdef __BIG_ENDIAN
> > +   switch (bpp) {
> > +   case 8:
> > +   fmt = DRM_FORMAT_C8;
> > +   break;
> > +   case 24:
> > +   fmt = DRM_FORMAT_BGR888;
> > +   break;
> 
> BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.

To 8bpp no, but it can easily apply to 24bpp. Same was as it applies to
16bpp. Neither matches the word size of the CPU or anything like that
but still the bytes have to stored in memory in some order.

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm: fourcc byteorder: add bigendian support to drm_mode_legacy_fb_format

[Michel Dänzer]

On 24/04/17 03:25 PM, Gerd Hoffmann wrote:
> Return correct fourcc codes on bigendian.  Drivers must be adapted to
> this change.
> 
> Signed-off-by: Gerd Hoffmann 

Just to reiterate, this won't work for the radeon driver, which programs
the GPU to use (effectively, per the current definition that these are
little endian GPU formats) DRM_FORMAT_XRGB with pre-R600 and
DRM_FORMAT_BGRX with >= R600.


> +#ifdef __BIG_ENDIAN
> + switch (bpp) {
> + case 8:
> + fmt = DRM_FORMAT_C8;
> + break;
> + case 24:
> + fmt = DRM_FORMAT_BGR888;
> + break;

BTW, endianness as a concept cannot apply to 8 or 24 bpp formats.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel