Linux-Amlogic Archive on lore.kernel.org
 help / color / Atom feed
From: Neil Armstrong <narmstrong@baylibre.com>
To: Ayan Halder <Ayan.Halder@arm.com>
Cc: "khilman@baylibre.com" <khilman@baylibre.com>, nd <nd@arm.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"linux-amlogic@lists.infradead.org"
	<linux-amlogic@lists.infradead.org>
Subject: Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
Date: Thu, 10 Oct 2019 15:41:15 +0200
Message-ID: <44f1771f-d640-f23d-995f-7bfcadd213bc@baylibre.com> (raw)
In-Reply-To: <20191010132601.GA10110@arm.com>

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 <narmstrong@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 ?

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 ?

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.

Limiting to ABGR8888 would discard like every non-Android renderers,
using AFBC, I'm not sure it's the kernels driver's responsibility.

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


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

  reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-10  9:25 [PATCH 0/7] drm/meson: add AFBC support Neil Armstrong
2019-10-10  9:25 ` [PATCH 1/7] drm/meson: add AFBC decoder registers for GXM and G12A Neil Armstrong
2019-10-10  9:25 ` [PATCH 2/7] drm/meson: store the framebuffer width for plane commit Neil Armstrong
2019-10-10  9:25 ` [PATCH 3/7] drm/meson: Add AFBCD module driver Neil Armstrong
2019-10-10  9:25 ` [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane Neil Armstrong
2019-10-10 13:26   ` Ayan Halder
2019-10-10 13:41     ` Neil Armstrong [this message]
2019-10-10 17:31       ` Ayan Halder
2019-10-11  7:46         ` Daniel Vetter
2019-10-11  7:56           ` Daniel Stone
2019-10-11  8:41       ` Brian Starkey
2019-10-11  9:14         ` Neil Armstrong
2019-10-11 10:56           ` Brian Starkey
2019-10-11 12:07             ` Neil Armstrong
2019-10-15 11:18               ` Brian Starkey
2019-10-15 11:46                 ` Neil Armstrong
2019-10-11 17:25             ` Daniel Vetter
2019-10-14  9:11               ` Brian Starkey
2019-10-14  9:20                 ` Daniel Vetter
2019-10-10  9:25 ` [PATCH 5/7] drm/meson: viu: add AFBC modules routing functions Neil Armstrong
2019-10-10  9:25 ` [PATCH 6/7] drm/meson: hold 32 lines after vsync to give time for AFBC start Neil Armstrong
2019-10-10  9:25 ` [PATCH 7/7] drm/meson: crtc: add OSD1 plane AFBC commit Neil Armstrong

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=44f1771f-d640-f23d-995f-7bfcadd213bc@baylibre.com \
    --to=narmstrong@baylibre.com \
    --cc=Ayan.Halder@arm.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=khilman@baylibre.com \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=nd@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Amlogic Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-amlogic/0 linux-amlogic/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-amlogic linux-amlogic/ https://lore.kernel.org/linux-amlogic \
		linux-amlogic@lists.infradead.org linux-amlogic@archiver.kernel.org
	public-inbox-index linux-amlogic

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-amlogic


AGPL code for this site: git clone https://public-inbox.org/ public-inbox