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 "0x1000000" 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 <narmstr...@baylibre.com>
> >> ---
> >> 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_XRGB8888:
> >> + case DRM_FORMAT_ARGB8888:
> >> + case DRM_FORMAT_XBGR8888:
> >> + case DRM_FORMAT_ABGR8888:
> > 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 ABGR8888 ?
>
> But why if the HW (GPU and DPU) is capable of ?
AFBC doesn't have an in-memory component order in the traditional
sense (i.e. a bit-position to component mapping), so Arm
have decided to define the convention that DRM_FORMAT_ABGR8888
represents the AFBC layout with R in component 0.
Are you sure the GPU supports other orders? I think any Arm driver
will only be producing DRM_FORMATs with "BGR" order e.g. ABGR8888.
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).
If you do choose to expose orders other than BGR/ABGR, then you should
certainly not allow YTR to be used with any orders other than
BGR/ABGR. The AFBC spec defines YTR as using R in component 0, which
Arm has defined as DRM_FORMAT_*BGR* (component 0 in LE LSBs).
>
> Isn't it an userspace choice ? I understand XRGB8888 is a waste
> of memory space and compression efficiency, but this is not the
> kernel driver's to decide this, right ?
>
As long as it's agreed and understood what XRGB8888 means. It must be
an AFBC bitstream with 4-components, with B in component 0, G in
component 1, R in component 2 and 8 wasted bits in component 3.
I know of HW which treats "XBGR" with AFBC as a 3-component format,
which isn't correct but can easily lead to confusion and
incompatibility.
> For interoperability I'll understand recommending a minimal set
> of modifiers and formats. But here, each platform is also limited
> by it's GPU capabilites aswell.
>
The (Arm) GPUs support ABGR ordering, so if everyone sticks to that we
can make sure everything's nice and compatible (until someone turns up
with HW which _doesn't_ support that ordering).
> Limiting to ABGR8888 would discard like every non-Android renderers,
> using AFBC, I'm not sure it's the kernels driver's responsibility.
>
It prevents renderers with hard-coded pixel formats, perhaps. But
those are already fragile by nature, surely?
Cheers,
-Brian
> >
> > 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_plane *plane,
> >> struct drm_plane_state *old_state)
> >> {
>
> [...]
>
> >>
> >> +static bool meson_plane_format_mod_supported(struct drm_plane *plane,
> >> + u32 format, u64 modifier)
> >> +{
> >> + struct meson_plane *meson_plane = to_meson_plane(plane);
> >> + struct meson_drm *priv = meson_plane->priv;
> >> + int i;
> >> +
> >> + if (modifier == DRM_FORMAT_MOD_INVALID)
> >> + return false;
> >> +
> >> + if (modifier == DRM_FORMAT_MOD_LINEAR)
> >> + return true;
> >> +
> >> + if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) &&
> >> + !meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> >> + return false;
> >> +
> >> + if (modifier & ~DRM_FORMAT_MOD_ARM_AFBC(MESON_MOD_AFBC_VALID_BITS))
> >> + return false;
> >> +
> >> + for (i = 0 ; i < plane->modifier_count ; ++i)
> >> + if (plane->modifiers[i] == modifier)
> >> + break;
> >> +
> >> + if (i == plane->modifier_count) {
> >> + DRM_DEBUG_KMS("Unsupported modifier\n");
> >> + return false;
> >> + }
>
> I can add a warn_once here, would it be enough ?
>
> >> +
> >> + if (priv->afbcd.ops && priv->afbcd.ops->supported_fmt)
> >> + return priv->afbcd.ops->supported_fmt(modifier, format);
> >> +
> >> + DRM_DEBUG_KMS("AFBC Unsupported\n");
> >> + return false;
> >> +}
> >> +
> >> static const struct drm_plane_funcs meson_plane_funcs = {
> >> .update_plane = drm_atomic_helper_update_plane,
> >> .disable_plane = drm_atomic_helper_disable_plane,
> >> @@ -353,6 +457,7 @@ static const struct drm_plane_funcs meson_plane_funcs
> >> = {
> >> .reset = drm_atomic_helper_plane_reset,
> >> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> >> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> >> + .format_mod_supported = meson_plane_format_mod_supported,
> >> };
> >>
> >> static const uint32_t supported_drm_formats[] = {
> >> @@ -364,10 +469,53 @@ static const uint32_t supported_drm_formats[] = {
> >> DRM_FORMAT_RGB565,
> >> };
> >>
> >> +static const uint64_t format_modifiers_afbc_gxm[] = {
> >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> >> + AFBC_FORMAT_MOD_SPARSE |
> >> + AFBC_FORMAT_MOD_YTR),
> >> + /* SPLIT mandates SPARSE, RGB modes mandates YTR */
> >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> >> + AFBC_FORMAT_MOD_YTR |
> >> + AFBC_FORMAT_MOD_SPARSE |
> >> + AFBC_FORMAT_MOD_SPLIT),
> >> + DRM_FORMAT_MOD_LINEAR,
> >> + DRM_FORMAT_MOD_INVALID,
> >> +};
> >> +
> >> +static const uint64_t format_modifiers_afbc_g12a[] = {
> >> + /*
> >> + * - TOFIX Support AFBC modifiers for YUV formats (16x16 + TILED)
> >> + * - AFBC_FORMAT_MOD_YTR is mandatory since we only support RGB
> >> + * - SPLIT is mandatory for performances reasons when in 16x16
> >> + * block size
> >> + * - 32x8 block size + SPLIT is mandatory with 4K frame size
> >> + * for performances reasons
> >> + */
> >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> >> + AFBC_FORMAT_MOD_YTR |
> >> + AFBC_FORMAT_MOD_SPARSE |
> >> + AFBC_FORMAT_MOD_SPLIT),
> >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
> >> + AFBC_FORMAT_MOD_YTR |
> >> + AFBC_FORMAT_MOD_SPARSE),
> >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
> >> + AFBC_FORMAT_MOD_YTR |
> >> + AFBC_FORMAT_MOD_SPARSE |
> >> + AFBC_FORMAT_MOD_SPLIT),
> >> + DRM_FORMAT_MOD_LINEAR,
> >> + DRM_FORMAT_MOD_INVALID,
> >> +};
> >> +
> >> +static const uint64_t format_modifiers_default[] = {
> >> + DRM_FORMAT_MOD_LINEAR,
> >> + DRM_FORMAT_MOD_INVALID,
> >> +};
> >> +
> >> int meson_plane_create(struct meson_drm *priv)
> >> {
> >> struct meson_plane *meson_plane;
> >> struct drm_plane *plane;
> >> + const uint64_t *format_modifiers = format_modifiers_default;
> >>
> >> meson_plane = devm_kzalloc(priv->drm->dev, sizeof(*meson_plane),
> >> GFP_KERNEL);
> >> @@ -377,11 +525,16 @@ int meson_plane_create(struct meson_drm *priv)
> >> meson_plane->priv = priv;
> >> plane = &meson_plane->base;
> >>
> >> + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM))
> >> + format_modifiers = format_modifiers_afbc_gxm;
> >> + else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> >> + format_modifiers = format_modifiers_afbc_g12a;
> >> +
> >> drm_universal_plane_init(priv->drm, plane, 0xFF,
> >> &meson_plane_funcs,
> >> supported_drm_formats,
> >> ARRAY_SIZE(supported_drm_formats),
> >> - NULL,
> >> + format_modifiers,
> >> DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
> >>
> >> drm_plane_helper_add(plane, &meson_plane_helper_funcs);
> >> --
> >> 2.22.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
