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] [thread-next>] [day] [month] [year] [list] 
Message-ID: <MN2PR12MB3789022F127910E8A3A00E51CB640@MN2PR12MB3789.namprd12.prod.outlook.com>
Date:   Thu, 9 Jul 2020 08:51:43 +0000
From:   Angelo Ribeiro <Angelo.Ribeiro@...opsys.com>
To:     Philippe CORNU <philippe.cornu@...com>,
        Yannick FERTRE <yannick.fertre@...com>,
        Benjamin GAIGNARD <benjamin.gaignard@...com>,
        "airlied@...ux.ie" <airlied@...ux.ie>,
        "daniel@...ll.ch" <daniel@...ll.ch>,
        "mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>,
        Alexandre TORGUE <alexandre.torgue@...com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-stm32@...md-mailman.stormreply.com" 
        <linux-stm32@...md-mailman.stormreply.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "pop.adrian61@...il.com" <pop.adrian61@...il.com>
CC:     Jose Abreu <Jose.Abreu@...opsys.com>,
        Gustavo Pimentel <Gustavo.Pimentel@...opsys.com>,
        Joao Pinto <Joao.Pinto@...opsys.com>
Subject: RE: [PATCH v2] drm/bridge: dw-mipi-dsi.c: Add VPG runtime config
 through debugfs

Hi Philippe,

From: Philippe CORNU <philippe.cornu@...com>
Date: Thu, Jul 09, 2020 at 08:56:10

> 
> On 7/8/20 7:08 PM, Angelo Ribeiro wrote:
> > Hi,
> > 
> > Is this patch good to go?
> > @daniel@...ll.ch, @Philippe CORNU
> > 
> > Was already tested by @Yannick FERTRE
> > and @Adrian Pop
> > on https://urldefense.com/v3/__https://lkml.org/lkml/2020/4/6/691__;!!A4F2R9G_pg!Kt4QZq004dTCJ3GJ6t6RIaJMBrP5tWWgTlboJo1ZICktSxRegGKtp1VxYM1i2PiM$  .
> > 
> > Thanks,
> > Angelo
> > 
> > From: Yannick
> > FERTRE <yannick.fertre@...com>
> > Date: Wed, Jun 24, 2020 at 16:35:04
> > 
> >> Hello Angelo,
> >> thanks for the patch.
> >> Tested-by: Yannick Fertre <yannick.fertre@...com>
> >> Tested OK on STM32MP1-DISCO, DSI v1.31
> >>
> >> Best regards
> >>
> >>
> >> On 4/6/20 3:49 PM, Angelo Ribeiro wrote:
> >>> Add support for the video pattern generator (VPG) BER pattern mode and
> >>> configuration in runtime.
> >>>
> >>> This enables using the debugfs interface to manipulate the VPG after
> >>> the pipeline is set.
> >>> Also, enables the usage of the VPG BER pattern.
> >>>
> >>> Changes in v2:
> >>>     - Added VID_MODE_VPG_MODE
> >>>     - Solved incompatible return type on __get and __set
> >>>
> >>> Reported-by: kbuild test robot <lkp@...el.com>
> >>> Reported-by: Adrian Pop <pop.adrian61@...il.com>
> >>> Cc: Gustavo Pimentel <gustavo.pimentel@...opsys.com>
> >>> Cc: Joao Pinto <jpinto@...opsys.com>
> >>> Cc: Jose Abreu <jose.abreu@...opsys.com>
> >>> Signed-off-by: Angelo Ribeiro <angelo.ribeiro@...opsys.com>
> >>> ---
> >>>    drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 98 ++++++++++++++++++++++++---
> >>>    1 file changed, 90 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> >>> index b18351b..9de3645 100644
> >>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> >>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> >>> @@ -91,6 +91,7 @@
> >>>    #define VID_MODE_TYPE_BURST			0x2
> >>>    #define VID_MODE_TYPE_MASK			0x3
> >>>    #define VID_MODE_VPG_ENABLE		BIT(16)
> >>> +#define VID_MODE_VPG_MODE		BIT(20)
> >>>    #define VID_MODE_VPG_HORIZONTAL		BIT(24)
> >>>    
> >>>    #define DSI_VID_PKT_SIZE		0x3c
> >>> @@ -221,6 +222,21 @@
> >>>    #define PHY_STATUS_TIMEOUT_US		10000
> >>>    #define CMD_PKT_STATUS_TIMEOUT_US	20000
> >>>    
> >>> +#ifdef CONFIG_DEBUG_FS
> >>> +#define VPG_DEFS(name, dsi) \
> >>> +	((void __force *)&((*dsi).vpg_defs.name))
> >>> +
> >>> +#define REGISTER(name, mask, dsi) \
> >>> +	{ #name, VPG_DEFS(name, dsi), mask, dsi }
> >>> +
> >>> +struct debugfs_entries {
> >>> +	const char				*name;
> >>> +	bool					*reg;
> >>> +	u32					mask;
> >>> +	struct dw_mipi_dsi			*dsi;
> >>> +};
> >>> +#endif /* CONFIG_DEBUG_FS */
> >>> +
> >>>    struct dw_mipi_dsi {
> >>>    	struct drm_bridge bridge;
> >>>    	struct mipi_dsi_host dsi_host;
> >>> @@ -238,9 +254,12 @@ struct dw_mipi_dsi {
> >>>    
> >>>    #ifdef CONFIG_DEBUG_FS
> >>>    	struct dentry *debugfs;
> >>> -
> >>> -	bool vpg;
> >>> -	bool vpg_horizontal;
> >>> +	struct debugfs_entries *debugfs_vpg;
> >>> +	struct {
> >>> +		bool vpg;
> >>> +		bool vpg_horizontal;
> >>> +		bool vpg_ber_pattern;
> >>> +	} vpg_defs;
> >>>    #endif /* CONFIG_DEBUG_FS */
> >>>    
> >>>    	struct dw_mipi_dsi *master; /* dual-dsi master ptr */
> >>> @@ -530,9 +549,11 @@ static void dw_mipi_dsi_video_mode_config(struct dw_mipi_dsi *dsi)
> >>>    		val |= VID_MODE_TYPE_NON_BURST_SYNC_EVENTS;
> >>>    
> >>>    #ifdef CONFIG_DEBUG_FS
> >>> -	if (dsi->vpg) {
> >>> +	if (dsi->vpg_defs.vpg) {
> >>>    		val |= VID_MODE_VPG_ENABLE;
> >>> -		val |= dsi->vpg_horizontal ? VID_MODE_VPG_HORIZONTAL : 0;
> >>> +		val |= dsi->vpg_defs.vpg_horizontal ?
> >>> +		       VID_MODE_VPG_HORIZONTAL : 0;
> >>> +		val |= dsi->vpg_defs.vpg_ber_pattern ? VID_MODE_VPG_MODE : 0;
> >>>    	}
> >>>    #endif /* CONFIG_DEBUG_FS */
> >>>    
> >>> @@ -961,6 +982,68 @@ static const struct drm_bridge_funcs dw_mipi_dsi_bridge_funcs = {
> >>>    
> >>>    #ifdef CONFIG_DEBUG_FS
> >>>    
> >>> +int dw_mipi_dsi_debugfs_write(void *data, u64 val)
> >>> +{
> >>> +	struct debugfs_entries *vpg = data;
> >>> +	struct dw_mipi_dsi *dsi;
> >>> +	u32 mode_cfg;
> >>> +
> >>> +	if (!vpg)
> >>> +		return -ENODEV;
> >>> +
> >>> +	dsi = vpg->dsi;
> >>> +
> >>> +	*vpg->reg = (bool)val;
> >>> +
> >>> +	mode_cfg = dsi_read(dsi, DSI_VID_MODE_CFG);
> >>> +
> >>> +	if (*vpg->reg)
> >>> +		mode_cfg |= vpg->mask;
> >>> +	else
> >>> +		mode_cfg &= ~vpg->mask;
> >>> +
> >>> +	dsi_write(dsi, DSI_VID_MODE_CFG, mode_cfg);
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +int dw_mipi_dsi_debugfs_show(void *data, u64 *val)
> >>> +{
> >>> +	struct debugfs_entries *vpg = data;
> >>> +
> >>> +	if (!vpg)
> >>> +		return -ENODEV;
> >>> +
> >>> +	*val = *vpg->reg;
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +DEFINE_DEBUGFS_ATTRIBUTE(fops_x32, dw_mipi_dsi_debugfs_show,
> >>> +			 dw_mipi_dsi_debugfs_write, "%llu\n");
> >>> +
> >>> +static void debugfs_create_files(void *data)
> >>> +{
> >>> +	struct dw_mipi_dsi *dsi = data;
> >>> +	struct debugfs_entries debugfs[] = {
> >>> +		REGISTER(vpg, VID_MODE_VPG_ENABLE, dsi),
> >>> +		REGISTER(vpg_horizontal, VID_MODE_VPG_HORIZONTAL, dsi),
> >>> +		REGISTER(vpg_ber_pattern, VID_MODE_VPG_MODE, dsi),
> >>> +	};
> >>> +	int i;
> >>> +
> >>> +	dsi->debugfs_vpg = kmalloc(sizeof(debugfs), GFP_KERNEL);
> >>> +	if (!dsi->debugfs_vpg)
> >>> +		return;
> >>> +
> >>> +	memcpy(dsi->debugfs_vpg, debugfs, sizeof(debugfs));
> >>> +
> >>> +	for (i = 0; i < ARRAY_SIZE(debugfs); i++)
> >>> +		debugfs_create_file(dsi->debugfs_vpg[i].name, 0644,
> >>> +				    dsi->debugfs, &dsi->debugfs_vpg[i],
> >>> +				    &fops_x32);
> >>> +}
> >>> +
> >>>    static void dw_mipi_dsi_debugfs_init(struct dw_mipi_dsi *dsi)
> >>>    {
> >>>    	dsi->debugfs = debugfs_create_dir(dev_name(dsi->dev), NULL);
> >>> @@ -969,14 +1052,13 @@ static void dw_mipi_dsi_debugfs_init(struct dw_mipi_dsi *dsi)
> >>>    		return;
> >>>    	}
> >>>    
> >>> -	debugfs_create_bool("vpg", 0660, dsi->debugfs, &dsi->vpg);
> >>> -	debugfs_create_bool("vpg_horizontal", 0660, dsi->debugfs,
> >>> -			    &dsi->vpg_horizontal);
> >>> +	debugfs_create_files(dsi);
> 
> Hi Angelo,
> And thank you for your patch.
> Could you please explain why you have "so many lines" for adding the ber 
> pattern, instead of these 4 lines :
> 
> +#define VID_MODE_VPG_MODE		BIT(20)
> +bool vpg_ber_pattern;
> +val |= dsi->vpg_ber_pattern ? VID_MODE_VPG_MODE : 0;
> +debugfs_create_bool("vpg_ber_pattern", 0660, dsi->debugfs, 
> &dsi->vpg_ber_pattern);
> 
> Many thanks
> Philippe :-)

Thank you for the review.

With this implementation you only need to set the debugfs file
to control the VPG. Since the internal VPG is used for debug 
purposes it eases the use during your development.

Thanks,
Angelo

> 
> >>>    }
> >>>    
> >>>    static void dw_mipi_dsi_debugfs_remove(struct dw_mipi_dsi *dsi)
> >>>    {
> >>>    	debugfs_remove_recursive(dsi->debugfs);
> >>> +	kfree(dsi->debugfs_vpg);
> >>>    }
> >>>    
> >>>    #else
> >>>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@...ts.freedesktop.org
> >> https://urldefense.com/v3/__https://lists.freedesktop.org/mailman/listinfo/dri-devel__;!!A4F2R9G_pg!PaD758-TpCHJcJG4biB5oM3WJXd1mTbLitD8K1qzSVQ4Z06nc__06MR_sz-ITMIl$
> > 
> > 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://urldefense.com/v3/__https://lists.freedesktop.org/mailman/listinfo/dri-devel__;!!A4F2R9G_pg!Kt4QZq004dTCJ3GJ6t6RIaJMBrP5tWWgTlboJo1ZICktSxRegGKtp1VxYH7H4Xjz$

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ