linux-kernel - Re: [PATCH 2/2] drm/tidss: Add support for AM62L display subsystem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <eab600f6-bfc2-489c-b384-5b620164a556@linux.dev>
Date: Sat, 18 Jan 2025 14:57:44 +0530
From: Aradhya Bhatia <aradhya.bhatia@...ux.dev>
To: Devarsh Thakkar <devarsht@...com>, jyri.sarha@....fi,
 tomi.valkeinen@...asonboard.com, airlied@...il.com,
 maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de,
 dri-devel@...ts.freedesktop.org, simona@...ll.ch,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, robh@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org
Cc: praneeth@...com, vigneshr@...com, s-jain1@...com, r-donadkar@...com,
 j-choudhary@...com, h-shenoy@...com
Subject: Re: [PATCH 2/2] drm/tidss: Add support for AM62L display subsystem

Hi Devarsh,

Thanks for the patches.

On 31/12/24 14:34, Devarsh Thakkar wrote:
> Enable display for AM62L DSS [1] which supports only a single display
> pipeline using a single overlay manager, single video port and a single
> video lite pipeline which does not support scaling.
> 
> The output of video port is routed to SoC boundary via DPI interface and
> the DPI signals from the video port are also routed to DSI Tx controller
> present within the SoC.
> 
> [1]: Section 11.7 (Display Subsystem and Peripherals)
> Link : https://www.ti.com/lit/pdf/sprujb4
> 
> Signed-off-by: Devarsh Thakkar <devarsht@...com>
> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 34 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/tidss/tidss_dispc.h |  2 ++
>  drivers/gpu/drm/tidss/tidss_drv.c   |  1 +
>  3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index cacb5f3d8085..cd322d60b825 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -376,6 +376,35 @@ const struct dispc_features dispc_am62a7_feats = {
>  	.vid_order = { 1, 0 },
>  };
>  
> +const struct dispc_features dispc_am62l_feats = {
> +	.max_pclk_khz = {
> +		[DISPC_VP_DPI] = 165000,

The TRM mentions that the max the VP PLL can go is 150MHz, so maybe you
might need to update this.

That said, as far as I understand, the IP itself can support 165 MHz,
and I am wondering, what would we do if there comes out a new SoC that
uses AM62L DSS as is, but just bumps up the PLL capacity to 165MHz.

It would be odd to have a ditto feats structure with just the frequency
updated.

> +	},
> +
> +	.subrev = DISPC_AM62L,
> +
> +	.common = "common",
> +	.common_regs = tidss_am65x_common_regs,

Also, I don't think we should utilize this as is.

The AM62L common regions is different, and is, at best, a subset of the
AM65x/AM62x common register space.

For example, registers VID_IRQ{STATUS, ENABLE}_0 have been dropped,
along with VP_IRQ{STATUS, ENABLE}_1.

- Which brings to my next concern...

> +
> +	.num_vps = 1,
> +	.vp_name = { "vp1" },
> +	.ovr_name = { "ovr1" },
> +	.vpclk_name =  { "vp1" },
> +	.vp_bus_type = { DISPC_VP_DPI },
> +
> +	.vp_feat = { .color = {
> +			.has_ctm = true,
> +			.gamma_size = 256,
> +			.gamma_type = TIDSS_GAMMA_8BIT,
> +		},
> +	},
> +
> +	.num_planes = 1,
> +	.vid_name = { "vidl1" },
> +	.vid_lite = { true },
> +	.vid_order = { 0 },

...

Usually, VID1 is the first video pipeline, while VIDL1 remains the
second. Which is why vid1 occupies the index 0, and vidl1 occupies 1 -
even from the hardware perspective.

While AM62L has only one video (lite) pipeline - vidl1, and there is no
vid1, the hardware still treats vidl1 at index 1.

For example, the TRM has defined DISPC_VID_IRQ{STATUS, ENABLE}_1 (and
not _0) in the common region.

So, the vid_order here should be 1, not 0.

Regards
Aradhya