Re: [PATCH v3 drm-dp 6/8] drm/hisilicon/hibmc: Add colorbar-cfg feature and its debugfs file

[Yongbang Shi <shiyongbang@huawei.com>]

> On Sat, Feb 22, 2025 at 10:50:59AM +0800, Yongbang Shi wrote:
>> From: Baihan Li <libaihan@huawei.com>
>>
>> DP controller can support generating a color bar signal over the
>> DisplayPort interface. This can be useful to check for possible DDR
>> or GPU problems, as the signal generator resides completely in the DP
>> block. Add debugfs file that controls colorbar generator.
>>
>> echo: config the color bar register to display
>> cat: print the color bar configuration
>>
>> Signed-off-by: Baihan Li <libaihan@huawei.com>
>> Signed-off-by: Yongbang Shi <shiyongbang@huawei.com>
>> ---
>> ChangeLog:
>> v2 -> v3:
>>    - rewrite the commit log, suggested by Dmitry Baryshkov.
>>    - move colorbar debugfs entry to this patch, suggested by Dmitry Baryshkov.
>>    - change binary format to integer format, suggested by Dmitry Baryshkov.
>> v1 -> v2:
>>    - add colorbar introduction in commit, suggested by Dmitry Baryshkov.
>>    - splittting colorbar and debugfs in different patches, suggested by Dmitry Baryshkov.
>>    - deleting edid decoder and its debugfs, suggested by Dmitry Baryshkov.
>>    - using debugfs_init() callback, suggested by Dmitry Baryshkov.
>> ---
>>   drivers/gpu/drm/hisilicon/hibmc/Makefile      |   3 +-
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c    |  43 ++++++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h    |  29 +++++
>>   drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h   |   3 +
>>   .../drm/hisilicon/hibmc/hibmc_drm_debugfs.c   | 100 ++++++++++++++++++
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_dp.c    |   1 +
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |   2 +
>>   7 files changed, 180 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> index 43de077d6769..1f65c683282f 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> @@ -1,5 +1,6 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_i2c.o \
>> -	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o dp/dp_serdes.o hibmc_drm_dp.o
>> +	       dp/dp_aux.o dp/dp_link.o dp/dp_hw.o dp/dp_serdes.o hibmc_drm_dp.o \
>> +	       hibmc_drm_debugfs.o
>>   
>>   obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc-drm.o
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> index 9d673f431a0e..a921b98dbf50 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.c
>> @@ -227,3 +227,46 @@ int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode)
>>   
>>   	return 0;
>>   }
>> +
>> +static const struct hibmc_dp_color_raw g_rgb_raw[] = {
>> +	{CBAR_COLOR_BAR, 0x000, 0x000, 0x000},
>> +	{CBAR_WHITE,     0xfff, 0xfff, 0xfff},
>> +	{CBAR_RED,       0xfff, 0x000, 0x000},
>> +	{CBAR_ORANGE,    0xfff, 0x800, 0x000},
>> +	{CBAR_YELLOW,    0xfff, 0xfff, 0x000},
>> +	{CBAR_GREEN,     0x000, 0xfff, 0x000},
>> +	{CBAR_CYAN,      0x000, 0x800, 0x800},
>> +	{CBAR_BLUE,      0x000, 0x000, 0xfff},
>> +	{CBAR_PURPLE,    0x800, 0x000, 0x800},
>> +	{CBAR_BLACK,     0x000, 0x000, 0x000},
>> +};
>> +
>> +void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg)
>> +{
>> +	struct hibmc_dp_dev *dp_dev = dp->dp_dev;
>> +	struct hibmc_dp_color_raw raw_data;
>> +
>> +	if (cfg->enable) {
>> +		hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(9),
>> +					 cfg->self_timing);
>> +		hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, GENMASK(8, 1),
>> +					 cfg->dynamic_rate);
>> +		if (cfg->pattern == CBAR_COLOR_BAR) {
>> +			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(10), 0);
>> +		} else {
>> +			raw_data = g_rgb_raw[cfg->pattern];
>> +			drm_dbg_dp(dp->drm_dev, "r:%x g:%x b:%x\n", raw_data.r_value,
>> +				   raw_data.g_value, raw_data.b_value);
>> +			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(10), 1);
>> +			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, GENMASK(23, 12),
>> +						 raw_data.r_value);
>> +			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL1, GENMASK(23, 12),
>> +						 raw_data.g_value);
>> +			hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL1, GENMASK(11, 0),
>> +						 raw_data.b_value);
>> +		}
>> +	}
>> +
>> +	hibmc_dp_reg_write_field(dp_dev, HIBMC_DP_COLOR_BAR_CTRL, BIT(0), cfg->enable);
>> +	writel(HIBMC_DP_SYNC_EN_MASK, dp_dev->base + HIBMC_DP_TIMING_SYNC_CTRL);
>> +}
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> index 53b6d0beecea..83a53dae8012 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_hw.h
>> @@ -14,6 +14,33 @@
>>   
>>   struct hibmc_dp_dev;
>>   
>> +enum hibmc_dp_cbar_pattern {
>> +	CBAR_COLOR_BAR,
>> +	CBAR_WHITE,
>> +	CBAR_RED,
>> +	CBAR_ORANGE,
>> +	CBAR_YELLOW,
>> +	CBAR_GREEN,
>> +	CBAR_CYAN,
>> +	CBAR_BLUE,
>> +	CBAR_PURPLE,
>> +	CBAR_BLACK,
>> +};
>> +
>> +struct hibmc_dp_color_raw {
>> +	enum hibmc_dp_cbar_pattern pattern;
>> +	u32 r_value;
>> +	u32 g_value;
>> +	u32 b_value;
>> +};
>> +
>> +struct hibmc_dp_cbar_cfg {
>> +	u8 enable;
>> +	u8 self_timing;
>> +	u8 dynamic_rate; /* 0:static, 1-255(frame):dynamic */
>> +	enum hibmc_dp_cbar_pattern pattern;
>> +};
>> +
>>   struct hibmc_dp {
>>   	struct hibmc_dp_dev *dp_dev;
>>   	struct drm_device *drm_dev;
>> @@ -21,10 +48,12 @@ struct hibmc_dp {
>>   	struct drm_connector connector;
>>   	void __iomem *mmio;
>>   	struct drm_dp_aux aux;
>> +	struct hibmc_dp_cbar_cfg cfg;
>>   };
>>   
>>   int hibmc_dp_hw_init(struct hibmc_dp *dp);
>>   int hibmc_dp_mode_set(struct hibmc_dp *dp, struct drm_display_mode *mode);
>>   void hibmc_dp_display_en(struct hibmc_dp *dp, bool enable);
>> +void hibmc_dp_set_cbar(struct hibmc_dp *dp, const struct hibmc_dp_cbar_cfg *cfg);
>>   
>>   #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> index b75ac13a5ead..4c388f633081 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/dp/dp_reg.h
>> @@ -67,6 +67,9 @@
>>   #define HIBMC_DP_CFG_STREAM_HTOTAL_SIZE		GENMASK(31, 16)
>>   #define HIBMC_DP_CFG_STREAM_HBLANK_SIZE		GENMASK(15, 0)
>>   
>> +#define HIBMC_DP_COLOR_BAR_CTRL			0x260
>> +#define HIBMC_DP_COLOR_BAR_CTRL1		0x264
>> +
>>   #define HIBMC_DP_TIMING_GEN_CONFIG0		0x26c
>>   #define HIBMC_DP_CFG_TIMING_GEN0_HACTIVE	GENMASK(31, 16)
>>   #define HIBMC_DP_CFG_TIMING_GEN0_HBLANK		GENMASK(15, 0)
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
>> new file mode 100644
>> index 000000000000..8d050a36946e
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_debugfs.c
>> @@ -0,0 +1,100 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +// Copyright (c) 2024 Hisilicon Limited.
>> +
>> +#include <linux/debugfs.h>
>> +#include <linux/device.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/pci.h>
>> +
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_file.h>
>> +#include <drm/drm_debugfs.h>
>> +#include <drm/drm_edid.h>
>> +
>> +#include "hibmc_drm_drv.h"
>> +
>> +#define MAX_BUF_SIZE 12
>> +
>> +static ssize_t hibmc_control_write(struct file *file, const char __user *user_buf,
>> +				   size_t count, loff_t *ppos)
>> +{
>> +	struct hibmc_drm_private *priv = file_inode(file)->i_private;
>> +	struct hibmc_dp_cbar_cfg *cfg = &priv->dp.cfg;
>> +	int ret, idx;
>> +	u8 buf[MAX_BUF_SIZE];
>> +
>> +	if (count >= MAX_BUF_SIZE)
>> +		return -EINVAL;
>> +
>> +	if (copy_from_user(buf, user_buf, count))
>> +		return -EFAULT;
>> +
>> +	buf[count] = '\0';
>> +
> There should be at least some documentation on the written values.

Okay!


>> +	if (sscanf(buf, "%hhu %hhu %hhu %u", &cfg->enable, &cfg->self_timing,
>> +		   &cfg->dynamic_rate, &cfg->pattern) != 4) {
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (cfg->pattern > 9 || cfg->enable > 1 || cfg->self_timing > 1)
>> +		return -EINVAL;
>> +
>> +	ret = drm_dev_enter(&priv->dev, &idx);
>> +	if (!ret)
>> +		return -ENODEV;
>> +
>> +	hibmc_dp_set_cbar(&priv->dp, cfg);
>> +
>> +	drm_dev_exit(idx);
>> +
>> +	return count;
>> +}