Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane

2019-10-15 Thread Neil Armstrong
On 15/10/2019 13:18, Brian Starkey wrote:
> Hi Neil,
> 
> On Fri, Oct 11, 2019 at 02:07:01PM +0200, Neil Armstrong wrote:
>> On 11/10/2019 12:56, Brian Starkey wrote:
>>> Hi,
>>>
>>> On Fri, Oct 11, 2019 at 11:14:43AM +0200, Neil Armstrong wrote:
 Hi Brian,

 On 11/10/2019 10:41, Brian Starkey wrote:
> Hi Neil,
>
> On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
> 
> [snip]
> 
 You'll have to tell us if the closed libMali handling AFBC would accept
 ARGB as format to render with AFBC enabled, if not you're right
 I'll discard XRGB/ARGB for AFBC buffers completely.

 But it the libMali chooses tt generate an ARGB buffer whatever
 ARGB/XRGB/ABGR888/XBGR888 is asked, then no I'll keep it that way.

>>>
>>> Yeah, I'll try and get clarity on this. It's not at all clear to me
>>> either. When you say "accept ARGB as format to render with AFBC
>>> enabled", which API are you referring to, just so I can be clear? Do
>>> you have an example of some code you're using to render AFBC with the
>>> GPU blob?
>>
>> Let's take kmscube using EGL and GBM.
>>
>> The buffer is allocated using gbm_surface_create_with_modifiers(),
>> but the gbm implementation is vendor specified.
>>
>> Then the surface is passed to eglCreateWindowSurface().
>> Then the format is matched using eglGetConfigAttrib() with the
>> returned configs.
>>
>> Here support on the gbm and EGL implementation.
>>
> 
> Is this a use-case that works on your platform today?

Amlogic gave ma a libMali for miniGBM with AFBC enabled, but I haven't
been able to enable AFBC yet.

> 
> I went and asked around. An Arm gbm implementation supporting AFBC
> will reject AFBC allocations for orders other than (DRM-convention)
> BGR.

I trust you on this point, thanks for asking around.

Neil

> 
> Thanks,
> -Brian
> 

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

Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane

2019-10-15 Thread Brian Starkey
Hi Neil,

On Fri, Oct 11, 2019 at 02:07:01PM +0200, Neil Armstrong wrote:
> On 11/10/2019 12:56, Brian Starkey wrote:
> > Hi,
> > 
> > On Fri, Oct 11, 2019 at 11:14:43AM +0200, Neil Armstrong wrote:
> >> Hi Brian,
> >>
> >> On 11/10/2019 10:41, Brian Starkey wrote:
> >>> Hi Neil,
> >>>
> >>> On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:

[snip]

> >> You'll have to tell us if the closed libMali handling AFBC would accept
> >> ARGB as format to render with AFBC enabled, if not you're right
> >> I'll discard XRGB/ARGB for AFBC buffers completely.
> >>
> >> But it the libMali chooses tt generate an ARGB buffer whatever
> >> ARGB/XRGB/ABGR888/XBGR888 is asked, then no I'll keep it that way.
> >>
> > 
> > Yeah, I'll try and get clarity on this. It's not at all clear to me
> > either. When you say "accept ARGB as format to render with AFBC
> > enabled", which API are you referring to, just so I can be clear? Do
> > you have an example of some code you're using to render AFBC with the
> > GPU blob?
> 
> Let's take kmscube using EGL and GBM.
> 
> The buffer is allocated using gbm_surface_create_with_modifiers(),
> but the gbm implementation is vendor specified.
> 
> Then the surface is passed to eglCreateWindowSurface().
> Then the format is matched using eglGetConfigAttrib() with the
> returned configs.
> 
> Here support on the gbm and EGL implementation.
> 

Is this a use-case that works on your platform today?

I went and asked around. An Arm gbm implementation supporting AFBC
will reject AFBC allocations for orders other than (DRM-convention)
BGR.

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

Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane

2019-10-14 Thread Daniel Vetter
On Mon, Oct 14, 2019 at 09:11:17AM +, Brian Starkey wrote:
> On Fri, Oct 11, 2019 at 07:25:02PM +0200, Daniel Vetter wrote:
> > On Fri, Oct 11, 2019 at 12:56 PM Brian Starkey  
> > wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Oct 11, 2019 at 11:14:43AM +0200, Neil Armstrong wrote:
> > > > Hi Brian,
> > > >
> > > > On 11/10/2019 10:41, Brian Starkey wrote:
> > 
> > > > > Are you sure the GPU supports other orders? I think any Arm driver
> > > > > will only be producing DRM_FORMATs with "BGR" order e.g. ABGR888>
> > > > > I'm not convinced the GPU HW actually supports any other order, but
> > > > > it's all rather confusing with texture swizzling. What I can tell you
> > > > > for sure is that it _does_ support BGR order (in DRM naming
> > > > > convention).
> > > >
> > > > Well, since the Bifrost Mali blobs are closed-source and delivered
> > > > by licensees, it's hard to define what is supported from a closed
> > > > GPU HW, closed SW implementation to a closed pixel format 
> > > > implementation.
> > > >
> > >
> > > I hear you. IMO the only way to make any of this clear is to publish
> > > reference data and tests which make sure implementations match each
> > > other. It's something I'm trying to make happen.
> > 
> > *cough* igt test with crc/writeback validation *cough*
> > 
> > And you don't even need anything that actually compresses. All you
> > need is the minimal enough AFBC knowledge to set up the metadata that
> > everything is uncompressed. We're doing that for our intel compression
> > formats already, and it works great in making sure that we have
> > end-to-end agreement on all the bits and ordering and everything.
> 
> Yeah this was my original thinking too. Sadly, a decent number of the
> AFBC parameters can't be tested with uncompressed data, as the codec
> kicks into a different mode there.

Hm right I guess some of the flags/parameters only matter if you deal with
actual compressed data. Still, better than nothing I guess - this should
at least catch stuff like color channels wired up the wrong way compared
to linear, and fun stuff like that.

> > Ofc
> > it doesn't validate the decoder, but that's not really igts job.
> > Should be possible to convince ARM to release that (as open source, in
> > igt), since it would be fairly valuable for the entire ecosystem here
> > ...
> 
> I fully agree about the value proposition.

I'll be waiting for patch from arm then :-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane

2019-10-14 Thread Brian Starkey
On Fri, Oct 11, 2019 at 07:25:02PM +0200, Daniel Vetter wrote:
> On Fri, Oct 11, 2019 at 12:56 PM Brian Starkey  wrote:
> >
> > Hi,
> >
> > On Fri, Oct 11, 2019 at 11:14:43AM +0200, Neil Armstrong wrote:
> > > Hi Brian,
> > >
> > > On 11/10/2019 10:41, Brian Starkey wrote:
> 
> > > > Are you sure the GPU supports other orders? I think any Arm driver
> > > > will only be producing DRM_FORMATs with "BGR" order e.g. ABGR888>
> > > > I'm not convinced the GPU HW actually supports any other order, but
> > > > it's all rather confusing with texture swizzling. What I can tell you
> > > > for sure is that it _does_ support BGR order (in DRM naming
> > > > convention).
> > >
> > > Well, since the Bifrost Mali blobs are closed-source and delivered
> > > by licensees, it's hard to define what is supported from a closed
> > > GPU HW, closed SW implementation to a closed pixel format implementation.
> > >
> >
> > I hear you. IMO the only way to make any of this clear is to publish
> > reference data and tests which make sure implementations match each
> > other. It's something I'm trying to make happen.
> 
> *cough* igt test with crc/writeback validation *cough*
> 
> And you don't even need anything that actually compresses. All you
> need is the minimal enough AFBC knowledge to set up the metadata that
> everything is uncompressed. We're doing that for our intel compression
> formats already, and it works great in making sure that we have
> end-to-end agreement on all the bits and ordering and everything.

Yeah this was my original thinking too. Sadly, a decent number of the
AFBC parameters can't be tested with uncompressed data, as the codec
kicks into a different mode there.

> Ofc
> it doesn't validate the decoder, but that's not really igts job.
> Should be possible to convince ARM to release that (as open source, in
> igt), since it would be fairly valuable for the entire ecosystem here
> ...

I fully agree about the value proposition.

-Brian

> -Daniel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane

2019-10-11 Thread Daniel Vetter
On Fri, Oct 11, 2019 at 12:56 PM Brian Starkey  wrote:
>
> Hi,
>
> On Fri, Oct 11, 2019 at 11:14:43AM +0200, Neil Armstrong wrote:
> > Hi Brian,
> >
> > On 11/10/2019 10:41, Brian Starkey wrote:

> > > Are you sure the GPU supports other orders? I think any Arm driver
> > > will only be producing DRM_FORMATs with "BGR" order e.g. ABGR888>
> > > I'm not convinced the GPU HW actually supports any other order, but
> > > it's all rather confusing with texture swizzling. What I can tell you
> > > for sure is that it _does_ support BGR order (in DRM naming
> > > convention).
> >
> > Well, since the Bifrost Mali blobs are closed-source and delivered
> > by licensees, it's hard to define what is supported from a closed
> > GPU HW, closed SW implementation to a closed pixel format implementation.
> >
>
> I hear you. IMO the only way to make any of this clear is to publish
> reference data and tests which make sure implementations match each
> other. It's something I'm trying to make happen.

*cough* igt test with crc/writeback validation *cough*

And you don't even need anything that actually compresses. All you
need is the minimal enough AFBC knowledge to set up the metadata that
everything is uncompressed. We're doing that for our intel compression
formats already, and it works great in making sure that we have
end-to-end agreement on all the bits and ordering and everything. Ofc
it doesn't validate the decoder, but that's not really igts job.
Should be possible to convince ARM to release that (as open source, in
igt), since it would be fairly valuable for the entire ecosystem here
...
-Daniel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane

2019-10-11 Thread Neil Armstrong
On 11/10/2019 12:56, Brian Starkey wrote:
> Hi,
> 
> On Fri, Oct 11, 2019 at 11:14:43AM +0200, Neil Armstrong wrote:
>> Hi Brian,
>>
>> On 11/10/2019 10:41, Brian Starkey wrote:
>>> Hi Neil,
>>>
>>> On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
 Hi Ayan,

 On 10/10/2019 15:26, Ayan Halder wrote:
> On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
>> This adds all the OSD configuration plumbing to support the AFBC decoders
>> path to display of the OSD1 plane.
>>
>> The Amlogic GXM and G12A AFBC decoders are integrated very differently.
>>
>> The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
>> because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
>>
>> On the other side, the Amlogic G12A AFBC decoder seems to be an external
>> IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
>> feeding the OSD1 VIU pixel input.
>> This uses a weird "0x100" internal HW physical address on both
>> sides to transfer the pixels.
>>
>> For Amlogic GXM, the supported pixel formats are the same as the normal
>> linear OSD1 mode.
>>
>> On the other side, Amlogic added support for all AFBC v1.2 formats for
>> the G12A AFBC integration.
>>
>> For simplicity, we stick to the already supported formats for now.
>>
>> Signed-off-by: Neil Armstrong 
>> ---
>>  drivers/gpu/drm/meson/meson_crtc.c  |   2 +
>>  drivers/gpu/drm/meson/meson_drv.h   |   4 +
>>  drivers/gpu/drm/meson/meson_plane.c | 215 
>>  3 files changed, 190 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/meson/meson_crtc.c 
>> b/drivers/gpu/drm/meson/meson_crtc.c
>> index 57ae1c13d1e6..d478fa232951 100644
>> --- a/drivers/gpu/drm/meson/meson_crtc.c
>> +++ b/drivers/gpu/drm/meson/meson_crtc.c
>> @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
>>  if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
>>  writel_relaxed(priv->viu.osd1_ctrl_stat,
>>  priv->io_base + 
>> _REG(VIU_OSD1_CTRL_STAT));
>> +writel_relaxed(priv->viu.osd1_ctrl_stat2,
>> +priv->io_base + 
>> _REG(VIU_OSD1_CTRL_STAT2));
>>  writel_relaxed(priv->viu.osd1_blk0_cfg[0],
>>  priv->io_base + 
>> _REG(VIU_OSD1_BLK0_CFG_W0));
>>  writel_relaxed(priv->viu.osd1_blk0_cfg[1],
>> diff --git a/drivers/gpu/drm/meson/meson_drv.h 
>> b/drivers/gpu/drm/meson/meson_drv.h
>> index 60f13c6f34e5..de25349be8aa 100644
>> --- a/drivers/gpu/drm/meson/meson_drv.h
>> +++ b/drivers/gpu/drm/meson/meson_drv.h
>> @@ -53,8 +53,12 @@ struct meson_drm {
>>  bool osd1_enabled;
>>  bool osd1_interlace;
>>  bool osd1_commit;
>> +bool osd1_afbcd;
>>  uint32_t osd1_ctrl_stat;
>> +uint32_t osd1_ctrl_stat2;
>>  uint32_t osd1_blk0_cfg[5];
>> +uint32_t osd1_blk1_cfg4;
>> +uint32_t osd1_blk2_cfg4;
>>  uint32_t osd1_addr;
>>  uint32_t osd1_stride;
>>  uint32_t osd1_height;
>> diff --git a/drivers/gpu/drm/meson/meson_plane.c 
>> b/drivers/gpu/drm/meson/meson_plane.c
>> index 5e798c276037..412941aa8402 100644
>> --- a/drivers/gpu/drm/meson/meson_plane.c
>> +++ b/drivers/gpu/drm/meson/meson_plane.c
>> @@ -23,6 +23,7 @@
>>  #include "meson_plane.h"
>>  #include "meson_registers.h"
>>  #include "meson_viu.h"
>> +#include "meson_osd_afbcd.h"
>>  
>>  /* OSD_SCI_WH_M1 */
>>  #define SCI_WH_M1_W(w)  FIELD_PREP(GENMASK(28, 16), w)
>> @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane 
>> *plane,
>> false, true);
>>  }
>>  
>> +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |   
>> \
>> +   AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
>> \
>> +   AFBC_FORMAT_MOD_YTR |
>> \
>> +   AFBC_FORMAT_MOD_SPARSE | 
>> \
>> +   AFBC_FORMAT_MOD_SPLIT)
>> +
>>  /* Takes a fixed 16.16 number and converts it to integer. */
>>  static inline int64_t fixed16_to_int(int64_t value)
>>  {
>>  return value >> 16;
>>  }
>>  
>> +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
>> +{
>> +u32 line_stride = 0;
>> +
>> +switch (priv->afbcd.format) {
>> +  

Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane

2019-10-11 Thread Brian Starkey
Hi,

On Fri, Oct 11, 2019 at 11:14:43AM +0200, Neil Armstrong wrote:
> Hi Brian,
> 
> On 11/10/2019 10:41, Brian Starkey wrote:
> > Hi Neil,
> > 
> > On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
> >> Hi Ayan,
> >>
> >> On 10/10/2019 15:26, Ayan Halder wrote:
> >>> On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
>  This adds all the OSD configuration plumbing to support the AFBC decoders
>  path to display of the OSD1 plane.
> 
>  The Amlogic GXM and G12A AFBC decoders are integrated very differently.
> 
>  The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
>  because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
> 
>  On the other side, the Amlogic G12A AFBC decoder seems to be an external
>  IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
>  feeding the OSD1 VIU pixel input.
>  This uses a weird "0x100" internal HW physical address on both
>  sides to transfer the pixels.
> 
>  For Amlogic GXM, the supported pixel formats are the same as the normal
>  linear OSD1 mode.
> 
>  On the other side, Amlogic added support for all AFBC v1.2 formats for
>  the G12A AFBC integration.
> 
>  For simplicity, we stick to the already supported formats for now.
> 
>  Signed-off-by: Neil Armstrong 
>  ---
>   drivers/gpu/drm/meson/meson_crtc.c  |   2 +
>   drivers/gpu/drm/meson/meson_drv.h   |   4 +
>   drivers/gpu/drm/meson/meson_plane.c | 215 
>   3 files changed, 190 insertions(+), 31 deletions(-)
> 
>  diff --git a/drivers/gpu/drm/meson/meson_crtc.c 
>  b/drivers/gpu/drm/meson/meson_crtc.c
>  index 57ae1c13d1e6..d478fa232951 100644
>  --- a/drivers/gpu/drm/meson/meson_crtc.c
>  +++ b/drivers/gpu/drm/meson/meson_crtc.c
>  @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
>   if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
>   writel_relaxed(priv->viu.osd1_ctrl_stat,
>   priv->io_base + 
>  _REG(VIU_OSD1_CTRL_STAT));
>  +writel_relaxed(priv->viu.osd1_ctrl_stat2,
>  +priv->io_base + 
>  _REG(VIU_OSD1_CTRL_STAT2));
>   writel_relaxed(priv->viu.osd1_blk0_cfg[0],
>   priv->io_base + 
>  _REG(VIU_OSD1_BLK0_CFG_W0));
>   writel_relaxed(priv->viu.osd1_blk0_cfg[1],
>  diff --git a/drivers/gpu/drm/meson/meson_drv.h 
>  b/drivers/gpu/drm/meson/meson_drv.h
>  index 60f13c6f34e5..de25349be8aa 100644
>  --- a/drivers/gpu/drm/meson/meson_drv.h
>  +++ b/drivers/gpu/drm/meson/meson_drv.h
>  @@ -53,8 +53,12 @@ struct meson_drm {
>   bool osd1_enabled;
>   bool osd1_interlace;
>   bool osd1_commit;
>  +bool osd1_afbcd;
>   uint32_t osd1_ctrl_stat;
>  +uint32_t osd1_ctrl_stat2;
>   uint32_t osd1_blk0_cfg[5];
>  +uint32_t osd1_blk1_cfg4;
>  +uint32_t osd1_blk2_cfg4;
>   uint32_t osd1_addr;
>   uint32_t osd1_stride;
>   uint32_t osd1_height;
>  diff --git a/drivers/gpu/drm/meson/meson_plane.c 
>  b/drivers/gpu/drm/meson/meson_plane.c
>  index 5e798c276037..412941aa8402 100644
>  --- a/drivers/gpu/drm/meson/meson_plane.c
>  +++ b/drivers/gpu/drm/meson/meson_plane.c
>  @@ -23,6 +23,7 @@
>   #include "meson_plane.h"
>   #include "meson_registers.h"
>   #include "meson_viu.h"
>  +#include "meson_osd_afbcd.h"
>   
>   /* OSD_SCI_WH_M1 */
>   #define SCI_WH_M1_W(w)  FIELD_PREP(GENMASK(28, 16), w)
>  @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane 
>  *plane,
>  false, true);
>   }
>   
>  +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |   
>  \
>  +   AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
>  \
>  +   AFBC_FORMAT_MOD_YTR |
>  \
>  +   AFBC_FORMAT_MOD_SPARSE | 
>  \
>  +   AFBC_FORMAT_MOD_SPLIT)
>  +
>   /* Takes a fixed 16.16 number and converts it to integer. */
>   static inline int64_t fixed16_to_int(int64_t value)
>   {
>   return value >> 16;
>   }
>   
>  +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
>  +{
>  +u32 line_stride = 0;
>  +
>  +switch (priv->afbcd.format) {
>  +case DRM_FORMAT_RGB565:
>  +   

Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane

2019-10-11 Thread Neil Armstrong
Hi Brian,

On 11/10/2019 10:41, Brian Starkey wrote:
> Hi Neil,
> 
> On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
>> Hi Ayan,
>>
>> On 10/10/2019 15:26, Ayan Halder wrote:
>>> On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
 This adds all the OSD configuration plumbing to support the AFBC decoders
 path to display of the OSD1 plane.

 The Amlogic GXM and G12A AFBC decoders are integrated very differently.

 The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
 because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.

 On the other side, the Amlogic G12A AFBC decoder seems to be an external
 IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
 feeding the OSD1 VIU pixel input.
 This uses a weird "0x100" internal HW physical address on both
 sides to transfer the pixels.

 For Amlogic GXM, the supported pixel formats are the same as the normal
 linear OSD1 mode.

 On the other side, Amlogic added support for all AFBC v1.2 formats for
 the G12A AFBC integration.

 For simplicity, we stick to the already supported formats for now.

 Signed-off-by: Neil Armstrong 
 ---
  drivers/gpu/drm/meson/meson_crtc.c  |   2 +
  drivers/gpu/drm/meson/meson_drv.h   |   4 +
  drivers/gpu/drm/meson/meson_plane.c | 215 
  3 files changed, 190 insertions(+), 31 deletions(-)

 diff --git a/drivers/gpu/drm/meson/meson_crtc.c 
 b/drivers/gpu/drm/meson/meson_crtc.c
 index 57ae1c13d1e6..d478fa232951 100644
 --- a/drivers/gpu/drm/meson/meson_crtc.c
 +++ b/drivers/gpu/drm/meson/meson_crtc.c
 @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
writel_relaxed(priv->viu.osd1_ctrl_stat,
priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
 +  writel_relaxed(priv->viu.osd1_ctrl_stat2,
 +  priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
writel_relaxed(priv->viu.osd1_blk0_cfg[0],
priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
writel_relaxed(priv->viu.osd1_blk0_cfg[1],
 diff --git a/drivers/gpu/drm/meson/meson_drv.h 
 b/drivers/gpu/drm/meson/meson_drv.h
 index 60f13c6f34e5..de25349be8aa 100644
 --- a/drivers/gpu/drm/meson/meson_drv.h
 +++ b/drivers/gpu/drm/meson/meson_drv.h
 @@ -53,8 +53,12 @@ struct meson_drm {
bool osd1_enabled;
bool osd1_interlace;
bool osd1_commit;
 +  bool osd1_afbcd;
uint32_t osd1_ctrl_stat;
 +  uint32_t osd1_ctrl_stat2;
uint32_t osd1_blk0_cfg[5];
 +  uint32_t osd1_blk1_cfg4;
 +  uint32_t osd1_blk2_cfg4;
uint32_t osd1_addr;
uint32_t osd1_stride;
uint32_t osd1_height;
 diff --git a/drivers/gpu/drm/meson/meson_plane.c 
 b/drivers/gpu/drm/meson/meson_plane.c
 index 5e798c276037..412941aa8402 100644
 --- a/drivers/gpu/drm/meson/meson_plane.c
 +++ b/drivers/gpu/drm/meson/meson_plane.c
 @@ -23,6 +23,7 @@
  #include "meson_plane.h"
  #include "meson_registers.h"
  #include "meson_viu.h"
 +#include "meson_osd_afbcd.h"
  
  /* OSD_SCI_WH_M1 */
  #define SCI_WH_M1_W(w)FIELD_PREP(GENMASK(28, 16), w)
 @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane 
 *plane,
   false, true);
  }
  
 +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | 
 \
 + AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |\
 + AFBC_FORMAT_MOD_YTR |\
 + AFBC_FORMAT_MOD_SPARSE | \
 + AFBC_FORMAT_MOD_SPLIT)
 +
  /* Takes a fixed 16.16 number and converts it to integer. */
  static inline int64_t fixed16_to_int(int64_t value)
  {
return value >> 16;
  }
  
 +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
 +{
 +  u32 line_stride = 0;
 +
 +  switch (priv->afbcd.format) {
 +  case DRM_FORMAT_RGB565:
 +  line_stride = ((priv->viu.osd1_width << 4) + 127) >> 7;
 +  break;
 +  case DRM_FORMAT_RGB888:
 +  case DRM_FORMAT_XRGB:
 +  case DRM_FORMAT_ARGB:
 +  case DRM_FORMAT_XBGR:
 +  case DRM_FORMAT_ABGR:
>>> Please have a look at
>>> https://www.kernel.org/doc/html/latest/gpu/afbc.html for our
>>> recommendation. We suggest that *X* formats are avoided.
>>>
>>> Also, for interoperability and maximum compression efficiency (with
>>> AFBC_FORMAT_MOD_YTR), we sugges

Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane

2019-10-11 Thread Brian Starkey
Hi Neil,

On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
> Hi Ayan,
> 
> On 10/10/2019 15:26, Ayan Halder wrote:
> > On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
> >> This adds all the OSD configuration plumbing to support the AFBC decoders
> >> path to display of the OSD1 plane.
> >>
> >> The Amlogic GXM and G12A AFBC decoders are integrated very differently.
> >>
> >> The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
> >> because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
> >>
> >> On the other side, the Amlogic G12A AFBC decoder seems to be an external
> >> IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
> >> feeding the OSD1 VIU pixel input.
> >> This uses a weird "0x100" internal HW physical address on both
> >> sides to transfer the pixels.
> >>
> >> For Amlogic GXM, the supported pixel formats are the same as the normal
> >> linear OSD1 mode.
> >>
> >> On the other side, Amlogic added support for all AFBC v1.2 formats for
> >> the G12A AFBC integration.
> >>
> >> For simplicity, we stick to the already supported formats for now.
> >>
> >> Signed-off-by: Neil Armstrong 
> >> ---
> >>  drivers/gpu/drm/meson/meson_crtc.c  |   2 +
> >>  drivers/gpu/drm/meson/meson_drv.h   |   4 +
> >>  drivers/gpu/drm/meson/meson_plane.c | 215 
> >>  3 files changed, 190 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/meson/meson_crtc.c 
> >> b/drivers/gpu/drm/meson/meson_crtc.c
> >> index 57ae1c13d1e6..d478fa232951 100644
> >> --- a/drivers/gpu/drm/meson/meson_crtc.c
> >> +++ b/drivers/gpu/drm/meson/meson_crtc.c
> >> @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
> >>if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
> >>writel_relaxed(priv->viu.osd1_ctrl_stat,
> >>priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
> >> +  writel_relaxed(priv->viu.osd1_ctrl_stat2,
> >> +  priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
> >>writel_relaxed(priv->viu.osd1_blk0_cfg[0],
> >>priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
> >>writel_relaxed(priv->viu.osd1_blk0_cfg[1],
> >> diff --git a/drivers/gpu/drm/meson/meson_drv.h 
> >> b/drivers/gpu/drm/meson/meson_drv.h
> >> index 60f13c6f34e5..de25349be8aa 100644
> >> --- a/drivers/gpu/drm/meson/meson_drv.h
> >> +++ b/drivers/gpu/drm/meson/meson_drv.h
> >> @@ -53,8 +53,12 @@ struct meson_drm {
> >>bool osd1_enabled;
> >>bool osd1_interlace;
> >>bool osd1_commit;
> >> +  bool osd1_afbcd;
> >>uint32_t osd1_ctrl_stat;
> >> +  uint32_t osd1_ctrl_stat2;
> >>uint32_t osd1_blk0_cfg[5];
> >> +  uint32_t osd1_blk1_cfg4;
> >> +  uint32_t osd1_blk2_cfg4;
> >>uint32_t osd1_addr;
> >>uint32_t osd1_stride;
> >>uint32_t osd1_height;
> >> diff --git a/drivers/gpu/drm/meson/meson_plane.c 
> >> b/drivers/gpu/drm/meson/meson_plane.c
> >> index 5e798c276037..412941aa8402 100644
> >> --- a/drivers/gpu/drm/meson/meson_plane.c
> >> +++ b/drivers/gpu/drm/meson/meson_plane.c
> >> @@ -23,6 +23,7 @@
> >>  #include "meson_plane.h"
> >>  #include "meson_registers.h"
> >>  #include "meson_viu.h"
> >> +#include "meson_osd_afbcd.h"
> >>  
> >>  /* OSD_SCI_WH_M1 */
> >>  #define SCI_WH_M1_W(w)FIELD_PREP(GENMASK(28, 16), w)
> >> @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane 
> >> *plane,
> >>   false, true);
> >>  }
> >>  
> >> +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | 
> >> \
> >> + AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |\
> >> + AFBC_FORMAT_MOD_YTR |\
> >> + AFBC_FORMAT_MOD_SPARSE | \
> >> + AFBC_FORMAT_MOD_SPLIT)
> >> +
> >>  /* Takes a fixed 16.16 number and converts it to integer. */
> >>  static inline int64_t fixed16_to_int(int64_t value)
> >>  {
> >>return value >> 16;
> >>  }
> >>  
> >> +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
> >> +{
> >> +  u32 line_stride = 0;
> >> +
> >> +  switch (priv->afbcd.format) {
> >> +  case DRM_FORMAT_RGB565:
> >> +  line_stride = ((priv->viu.osd1_width << 4) + 127) >> 7;
> >> +  break;
> >> +  case DRM_FORMAT_RGB888:
> >> +  case DRM_FORMAT_XRGB:
> >> +  case DRM_FORMAT_ARGB:
> >> +  case DRM_FORMAT_XBGR:
> >> +  case DRM_FORMAT_ABGR:
> > Please have a look at
> > https://www.kernel.org/doc/html/latest/gpu/afbc.html for our
> > recommendation. We suggest that *X* formats are avoided.
> > 
> > Also, for interoperability and maximum compression efficiency (with
> > AFBC_FORMAT_MOD_YTR), we suggest the following order :-
> > 
> > Component 0: R
> >

Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane

2019-10-11 Thread Daniel Stone
Hi,

On Fri, 11 Oct 2019 at 08:46, Daniel Vetter  wrote:
> On Thu, Oct 10, 2019 at 7:32 PM Ayan Halder  wrote:
> > On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
> > > Sorry I don't understand, you ask me to limit AFBC to ABGR ?
> >
> > Apologies for the confusion, as per the link, the formats which are
> > suggested with AFBC_FORMAT_MOD_YTR are the BGR/ABGR formats (as
> > listed in the 'AFBC formats' table)
> >
> > Thus, any other permutation of the components might make it incompatible
> > with some other AFBC producers/consumers.
>
> Uh, that's not how this is supposed to be used. Drivers are meant to
> expose _everything_ they support (bonus if you roughly sort it in
> preference order). Userspace then computes the intersection of
> modifiers/formats supported by all devices it needs to share a buffer
> with. Allowing that was the entire point of modifiers, if we
> artificially limit to the common denominator we're back "only linear
> works everywhere".

Correct.

A lot of userspace will select for format first, then find a modifier
which can be used with that format. If userspace has specific
knowledge of AFBC and decides that it prefers to use AFBC so will seek
out an alpha format - and make sure everyone fills the channel solid -
then that's fine. But that's putting the cart before the horse.

Not exposing XRGB on the primary plane will break a lot of
userspace, so in this case AFBC would just never really be used.

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

Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane

2019-10-11 Thread Daniel Vetter
On Thu, Oct 10, 2019 at 7:32 PM Ayan Halder  wrote:
>
> On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
> > Hi Ayan,
> >
> > On 10/10/2019 15:26, Ayan Halder wrote:
> > > On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
> > >> This adds all the OSD configuration plumbing to support the AFBC decoders
> > >> path to display of the OSD1 plane.
> > >>
> > >> The Amlogic GXM and G12A AFBC decoders are integrated very differently.
> > >>
> > >> The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
> > >> because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
> > >>
> > >> On the other side, the Amlogic G12A AFBC decoder seems to be an external
> > >> IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
> > >> feeding the OSD1 VIU pixel input.
> > >> This uses a weird "0x100" internal HW physical address on both
> > >> sides to transfer the pixels.
> > >>
> > >> For Amlogic GXM, the supported pixel formats are the same as the normal
> > >> linear OSD1 mode.
> > >>
> > >> On the other side, Amlogic added support for all AFBC v1.2 formats for
> > >> the G12A AFBC integration.
> > >>
> > >> For simplicity, we stick to the already supported formats for now.
> > >>
> > >> Signed-off-by: Neil Armstrong 
> > >> ---
> > >>  drivers/gpu/drm/meson/meson_crtc.c  |   2 +
> > >>  drivers/gpu/drm/meson/meson_drv.h   |   4 +
> > >>  drivers/gpu/drm/meson/meson_plane.c | 215 
> > >>  3 files changed, 190 insertions(+), 31 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/meson/meson_crtc.c 
> > >> b/drivers/gpu/drm/meson/meson_crtc.c
> > >> index 57ae1c13d1e6..d478fa232951 100644
> > >> --- a/drivers/gpu/drm/meson/meson_crtc.c
> > >> +++ b/drivers/gpu/drm/meson/meson_crtc.c
> > >> @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
> > >>if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
> > >>writel_relaxed(priv->viu.osd1_ctrl_stat,
> > >>priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
> > >> +  writel_relaxed(priv->viu.osd1_ctrl_stat2,
> > >> +  priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
> > >>writel_relaxed(priv->viu.osd1_blk0_cfg[0],
> > >>priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
> > >>writel_relaxed(priv->viu.osd1_blk0_cfg[1],
> > >> diff --git a/drivers/gpu/drm/meson/meson_drv.h 
> > >> b/drivers/gpu/drm/meson/meson_drv.h
> > >> index 60f13c6f34e5..de25349be8aa 100644
> > >> --- a/drivers/gpu/drm/meson/meson_drv.h
> > >> +++ b/drivers/gpu/drm/meson/meson_drv.h
> > >> @@ -53,8 +53,12 @@ struct meson_drm {
> > >>bool osd1_enabled;
> > >>bool osd1_interlace;
> > >>bool osd1_commit;
> > >> +  bool osd1_afbcd;
> > >>uint32_t osd1_ctrl_stat;
> > >> +  uint32_t osd1_ctrl_stat2;
> > >>uint32_t osd1_blk0_cfg[5];
> > >> +  uint32_t osd1_blk1_cfg4;
> > >> +  uint32_t osd1_blk2_cfg4;
> > >>uint32_t osd1_addr;
> > >>uint32_t osd1_stride;
> > >>uint32_t osd1_height;
> > >> diff --git a/drivers/gpu/drm/meson/meson_plane.c 
> > >> b/drivers/gpu/drm/meson/meson_plane.c
> > >> index 5e798c276037..412941aa8402 100644
> > >> --- a/drivers/gpu/drm/meson/meson_plane.c
> > >> +++ b/drivers/gpu/drm/meson/meson_plane.c
> > >> @@ -23,6 +23,7 @@
> > >>  #include "meson_plane.h"
> > >>  #include "meson_registers.h"
> > >>  #include "meson_viu.h"
> > >> +#include "meson_osd_afbcd.h"
> > >>
> > >>  /* OSD_SCI_WH_M1 */
> > >>  #define SCI_WH_M1_W(w)FIELD_PREP(GENMASK(28, 16), w)
> > >> @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane 
> > >> *plane,
> > >>   false, true);
> > >>  }
> > >>
> > >> +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |   
> > >>   \
> > >> + AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |\
> > >> + AFBC_FORMAT_MOD_YTR |\
> > >> + AFBC_FORMAT_MOD_SPARSE | \
> > >> + AFBC_FORMAT_MOD_SPLIT)
> > >> +
> > >>  /* Takes a fixed 16.16 number and converts it to integer. */
> > >>  static inline int64_t fixed16_to_int(int64_t value)
> > >>  {
> > >>return value >> 16;
> > >>  }
> > >>
> > >> +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
> > >> +{
> > >> +  u32 line_stride = 0;
> > >> +
> > >> +  switch (priv->afbcd.format) {
> > >> +  case DRM_FORMAT_RGB565:
> > >> +  line_stride = ((priv->viu.osd1_width << 4) + 127) >> 7;
> > >> +  break;
> > >> +  case DRM_FORMAT_RGB888:
> > >> +  case DRM_FORMAT_XRGB:
> > >> +  case DRM_FORMAT_ARGB:
> > >> +  case DRM_FORMAT_XBGR:
> > >> +  case DRM_FORMAT_ABGR:
> > > Please have a look at
> > > https://www.kernel.org/doc/html/l

Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane

2019-10-10 Thread Ayan Halder
On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
> Hi Ayan,
> 
> On 10/10/2019 15:26, Ayan Halder wrote:
> > On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
> >> This adds all the OSD configuration plumbing to support the AFBC decoders
> >> path to display of the OSD1 plane.
> >>
> >> The Amlogic GXM and G12A AFBC decoders are integrated very differently.
> >>
> >> The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
> >> because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
> >>
> >> On the other side, the Amlogic G12A AFBC decoder seems to be an external
> >> IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
> >> feeding the OSD1 VIU pixel input.
> >> This uses a weird "0x100" internal HW physical address on both
> >> sides to transfer the pixels.
> >>
> >> For Amlogic GXM, the supported pixel formats are the same as the normal
> >> linear OSD1 mode.
> >>
> >> On the other side, Amlogic added support for all AFBC v1.2 formats for
> >> the G12A AFBC integration.
> >>
> >> For simplicity, we stick to the already supported formats for now.
> >>
> >> Signed-off-by: Neil Armstrong 
> >> ---
> >>  drivers/gpu/drm/meson/meson_crtc.c  |   2 +
> >>  drivers/gpu/drm/meson/meson_drv.h   |   4 +
> >>  drivers/gpu/drm/meson/meson_plane.c | 215 
> >>  3 files changed, 190 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/meson/meson_crtc.c 
> >> b/drivers/gpu/drm/meson/meson_crtc.c
> >> index 57ae1c13d1e6..d478fa232951 100644
> >> --- a/drivers/gpu/drm/meson/meson_crtc.c
> >> +++ b/drivers/gpu/drm/meson/meson_crtc.c
> >> @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
> >>if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
> >>writel_relaxed(priv->viu.osd1_ctrl_stat,
> >>priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
> >> +  writel_relaxed(priv->viu.osd1_ctrl_stat2,
> >> +  priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
> >>writel_relaxed(priv->viu.osd1_blk0_cfg[0],
> >>priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
> >>writel_relaxed(priv->viu.osd1_blk0_cfg[1],
> >> diff --git a/drivers/gpu/drm/meson/meson_drv.h 
> >> b/drivers/gpu/drm/meson/meson_drv.h
> >> index 60f13c6f34e5..de25349be8aa 100644
> >> --- a/drivers/gpu/drm/meson/meson_drv.h
> >> +++ b/drivers/gpu/drm/meson/meson_drv.h
> >> @@ -53,8 +53,12 @@ struct meson_drm {
> >>bool osd1_enabled;
> >>bool osd1_interlace;
> >>bool osd1_commit;
> >> +  bool osd1_afbcd;
> >>uint32_t osd1_ctrl_stat;
> >> +  uint32_t osd1_ctrl_stat2;
> >>uint32_t osd1_blk0_cfg[5];
> >> +  uint32_t osd1_blk1_cfg4;
> >> +  uint32_t osd1_blk2_cfg4;
> >>uint32_t osd1_addr;
> >>uint32_t osd1_stride;
> >>uint32_t osd1_height;
> >> diff --git a/drivers/gpu/drm/meson/meson_plane.c 
> >> b/drivers/gpu/drm/meson/meson_plane.c
> >> index 5e798c276037..412941aa8402 100644
> >> --- a/drivers/gpu/drm/meson/meson_plane.c
> >> +++ b/drivers/gpu/drm/meson/meson_plane.c
> >> @@ -23,6 +23,7 @@
> >>  #include "meson_plane.h"
> >>  #include "meson_registers.h"
> >>  #include "meson_viu.h"
> >> +#include "meson_osd_afbcd.h"
> >>  
> >>  /* OSD_SCI_WH_M1 */
> >>  #define SCI_WH_M1_W(w)FIELD_PREP(GENMASK(28, 16), w)
> >> @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane 
> >> *plane,
> >>   false, true);
> >>  }
> >>  
> >> +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | 
> >> \
> >> + AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |\
> >> + AFBC_FORMAT_MOD_YTR |\
> >> + AFBC_FORMAT_MOD_SPARSE | \
> >> + AFBC_FORMAT_MOD_SPLIT)
> >> +
> >>  /* Takes a fixed 16.16 number and converts it to integer. */
> >>  static inline int64_t fixed16_to_int(int64_t value)
> >>  {
> >>return value >> 16;
> >>  }
> >>  
> >> +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
> >> +{
> >> +  u32 line_stride = 0;
> >> +
> >> +  switch (priv->afbcd.format) {
> >> +  case DRM_FORMAT_RGB565:
> >> +  line_stride = ((priv->viu.osd1_width << 4) + 127) >> 7;
> >> +  break;
> >> +  case DRM_FORMAT_RGB888:
> >> +  case DRM_FORMAT_XRGB:
> >> +  case DRM_FORMAT_ARGB:
> >> +  case DRM_FORMAT_XBGR:
> >> +  case DRM_FORMAT_ABGR:
> > Please have a look at
> > https://www.kernel.org/doc/html/latest/gpu/afbc.html for our
> > recommendation. We suggest that *X* formats are avoided.
> > 
> > Also, for interoperability and maximum compression efficiency (with
> > AFBC_FORMAT_MOD_YTR), we suggest the following order :-
> > 
> > Component 0: R
> > C

Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane

2019-10-10 Thread Neil Armstrong
Hi Ayan,

On 10/10/2019 15:26, Ayan Halder wrote:
> On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
>> This adds all the OSD configuration plumbing to support the AFBC decoders
>> path to display of the OSD1 plane.
>>
>> The Amlogic GXM and G12A AFBC decoders are integrated very differently.
>>
>> The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
>> because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
>>
>> On the other side, the Amlogic G12A AFBC decoder seems to be an external
>> IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
>> feeding the OSD1 VIU pixel input.
>> This uses a weird "0x100" internal HW physical address on both
>> sides to transfer the pixels.
>>
>> For Amlogic GXM, the supported pixel formats are the same as the normal
>> linear OSD1 mode.
>>
>> On the other side, Amlogic added support for all AFBC v1.2 formats for
>> the G12A AFBC integration.
>>
>> For simplicity, we stick to the already supported formats for now.
>>
>> Signed-off-by: Neil Armstrong 
>> ---
>>  drivers/gpu/drm/meson/meson_crtc.c  |   2 +
>>  drivers/gpu/drm/meson/meson_drv.h   |   4 +
>>  drivers/gpu/drm/meson/meson_plane.c | 215 
>>  3 files changed, 190 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/meson/meson_crtc.c 
>> b/drivers/gpu/drm/meson/meson_crtc.c
>> index 57ae1c13d1e6..d478fa232951 100644
>> --- a/drivers/gpu/drm/meson/meson_crtc.c
>> +++ b/drivers/gpu/drm/meson/meson_crtc.c
>> @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
>>  if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
>>  writel_relaxed(priv->viu.osd1_ctrl_stat,
>>  priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
>> +writel_relaxed(priv->viu.osd1_ctrl_stat2,
>> +priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
>>  writel_relaxed(priv->viu.osd1_blk0_cfg[0],
>>  priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
>>  writel_relaxed(priv->viu.osd1_blk0_cfg[1],
>> diff --git a/drivers/gpu/drm/meson/meson_drv.h 
>> b/drivers/gpu/drm/meson/meson_drv.h
>> index 60f13c6f34e5..de25349be8aa 100644
>> --- a/drivers/gpu/drm/meson/meson_drv.h
>> +++ b/drivers/gpu/drm/meson/meson_drv.h
>> @@ -53,8 +53,12 @@ struct meson_drm {
>>  bool osd1_enabled;
>>  bool osd1_interlace;
>>  bool osd1_commit;
>> +bool osd1_afbcd;
>>  uint32_t osd1_ctrl_stat;
>> +uint32_t osd1_ctrl_stat2;
>>  uint32_t osd1_blk0_cfg[5];
>> +uint32_t osd1_blk1_cfg4;
>> +uint32_t osd1_blk2_cfg4;
>>  uint32_t osd1_addr;
>>  uint32_t osd1_stride;
>>  uint32_t osd1_height;
>> diff --git a/drivers/gpu/drm/meson/meson_plane.c 
>> b/drivers/gpu/drm/meson/meson_plane.c
>> index 5e798c276037..412941aa8402 100644
>> --- a/drivers/gpu/drm/meson/meson_plane.c
>> +++ b/drivers/gpu/drm/meson/meson_plane.c
>> @@ -23,6 +23,7 @@
>>  #include "meson_plane.h"
>>  #include "meson_registers.h"
>>  #include "meson_viu.h"
>> +#include "meson_osd_afbcd.h"
>>  
>>  /* OSD_SCI_WH_M1 */
>>  #define SCI_WH_M1_W(w)  FIELD_PREP(GENMASK(28, 16), w)
>> @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane 
>> *plane,
>> false, true);
>>  }
>>  
>> +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |   
>> \
>> +   AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |\
>> +   AFBC_FORMAT_MOD_YTR |\
>> +   AFBC_FORMAT_MOD_SPARSE | \
>> +   AFBC_FORMAT_MOD_SPLIT)
>> +
>>  /* Takes a fixed 16.16 number and converts it to integer. */
>>  static inline int64_t fixed16_to_int(int64_t value)
>>  {
>>  return value >> 16;
>>  }
>>  
>> +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
>> +{
>> +u32 line_stride = 0;
>> +
>> +switch (priv->afbcd.format) {
>> +case DRM_FORMAT_RGB565:
>> +line_stride = ((priv->viu.osd1_width << 4) + 127) >> 7;
>> +break;
>> +case DRM_FORMAT_RGB888:
>> +case DRM_FORMAT_XRGB:
>> +case DRM_FORMAT_ARGB:
>> +case DRM_FORMAT_XBGR:
>> +case DRM_FORMAT_ABGR:
> Please have a look at
> https://www.kernel.org/doc/html/latest/gpu/afbc.html for our
> recommendation. We suggest that *X* formats are avoided.
> 
> Also, for interoperability and maximum compression efficiency (with
> AFBC_FORMAT_MOD_YTR), we suggest the following order :-
> 
> Component 0: R
> Component 1: G
> Component 2: B
> Component 3: A (if available)


Sorry I don't understand, you ask me to limit AFBC to ABGR ?

But why if the HW (GPU and DPU) is capable of ?

Isn't it an userspace choice

Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane

2019-10-10 Thread Ayan Halder
On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
> This adds all the OSD configuration plumbing to support the AFBC decoders
> path to display of the OSD1 plane.
> 
> The Amlogic GXM and G12A AFBC decoders are integrated very differently.
> 
> The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
> because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
> 
> On the other side, the Amlogic G12A AFBC decoder seems to be an external
> IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
> feeding the OSD1 VIU pixel input.
> This uses a weird "0x100" internal HW physical address on both
> sides to transfer the pixels.
> 
> For Amlogic GXM, the supported pixel formats are the same as the normal
> linear OSD1 mode.
> 
> On the other side, Amlogic added support for all AFBC v1.2 formats for
> the G12A AFBC integration.
> 
> For simplicity, we stick to the already supported formats for now.
> 
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/gpu/drm/meson/meson_crtc.c  |   2 +
>  drivers/gpu/drm/meson/meson_drv.h   |   4 +
>  drivers/gpu/drm/meson/meson_plane.c | 215 
>  3 files changed, 190 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_crtc.c 
> b/drivers/gpu/drm/meson/meson_crtc.c
> index 57ae1c13d1e6..d478fa232951 100644
> --- a/drivers/gpu/drm/meson/meson_crtc.c
> +++ b/drivers/gpu/drm/meson/meson_crtc.c
> @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
>   if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
>   writel_relaxed(priv->viu.osd1_ctrl_stat,
>   priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
> + writel_relaxed(priv->viu.osd1_ctrl_stat2,
> + priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
>   writel_relaxed(priv->viu.osd1_blk0_cfg[0],
>   priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
>   writel_relaxed(priv->viu.osd1_blk0_cfg[1],
> diff --git a/drivers/gpu/drm/meson/meson_drv.h 
> b/drivers/gpu/drm/meson/meson_drv.h
> index 60f13c6f34e5..de25349be8aa 100644
> --- a/drivers/gpu/drm/meson/meson_drv.h
> +++ b/drivers/gpu/drm/meson/meson_drv.h
> @@ -53,8 +53,12 @@ struct meson_drm {
>   bool osd1_enabled;
>   bool osd1_interlace;
>   bool osd1_commit;
> + bool osd1_afbcd;
>   uint32_t osd1_ctrl_stat;
> + uint32_t osd1_ctrl_stat2;
>   uint32_t osd1_blk0_cfg[5];
> + uint32_t osd1_blk1_cfg4;
> + uint32_t osd1_blk2_cfg4;
>   uint32_t osd1_addr;
>   uint32_t osd1_stride;
>   uint32_t osd1_height;
> diff --git a/drivers/gpu/drm/meson/meson_plane.c 
> b/drivers/gpu/drm/meson/meson_plane.c
> index 5e798c276037..412941aa8402 100644
> --- a/drivers/gpu/drm/meson/meson_plane.c
> +++ b/drivers/gpu/drm/meson/meson_plane.c
> @@ -23,6 +23,7 @@
>  #include "meson_plane.h"
>  #include "meson_registers.h"
>  #include "meson_viu.h"
> +#include "meson_osd_afbcd.h"
>  
>  /* OSD_SCI_WH_M1 */
>  #define SCI_WH_M1_W(w)   FIELD_PREP(GENMASK(28, 16), w)
> @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane 
> *plane,
>  false, true);
>  }
>  
> +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> \
> +AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |\
> +AFBC_FORMAT_MOD_YTR |\
> +AFBC_FORMAT_MOD_SPARSE | \
> +AFBC_FORMAT_MOD_SPLIT)
> +
>  /* Takes a fixed 16.16 number and converts it to integer. */
>  static inline int64_t fixed16_to_int(int64_t value)
>  {
>   return value >> 16;
>  }
>  
> +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
> +{
> + u32 line_stride = 0;
> +
> + switch (priv->afbcd.format) {
> + case DRM_FORMAT_RGB565:
> + line_stride = ((priv->viu.osd1_width << 4) + 127) >> 7;
> + break;
> + case DRM_FORMAT_RGB888:
> + case DRM_FORMAT_XRGB:
> + case DRM_FORMAT_ARGB:
> + case DRM_FORMAT_XBGR:
> + case DRM_FORMAT_ABGR:
Please have a look at
https://www.kernel.org/doc/html/latest/gpu/afbc.html for our
recommendation. We suggest that *X* formats are avoided.

Also, for interoperability and maximum compression efficiency (with
AFBC_FORMAT_MOD_YTR), we suggest the following order :-

Component 0: R
Component 1: G
Component 2: B
Component 3: A (if available)

Thus, DRM_FORMAT_ABGR, DRM_FORMAT_BGR should only be allowed.
> + line_stride = ((priv->viu.osd1_width << 5) + 127) >> 7;
> + break;
> + }
> +
> + return ((line_stride + 1) >> 1) << 1;
> +}
> +
>  static void meson_plane_atomic_update(struct drm