Re: [PATCH v4 10/10] drm/ingenic: add some jz4780 specific features

["H. Nikolaus Schaller" <hns@goldelico.com>]

Hi Paul,

> Am 28.09.2021 um 11:58 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi,
> 
> Le lun., sept. 27 2021 at 18:44:28 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> From: Paul Boddie <paul@boddie.org.uk>
>> The jz4780 has some features which need initialization
>> according to the vendor kernel.
>> Signed-off-by: Paul Boddie <paul@boddie.org.uk>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 39 +++++++++++++++++++++++
>> 1 file changed, 39 insertions(+)
>> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> index e2df4b085905..605549b316b5 100644
>> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
>> @@ -66,6 +66,10 @@ struct jz_soc_info {
>> 	bool needs_dev_clk;
>> 	bool has_osd;
>> 	bool map_noncoherent;
>> +	bool has_alpha;
>> +	bool has_pcfg;
>> +	bool has_recover;
>> +	bool has_rgbc;
>> 	bool use_extended_hwdesc;
>> 	unsigned int max_width, max_height;
>> 	const u32 *formats_f0, *formats_f1;
>> @@ -732,6 +736,9 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>> 		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
>> 	}
>> +	if (priv->soc_info->has_recover)
>> +		cfg |= JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN;
> 
> Did you actually test this? I know that in theory it sounds like something we'd want, but unless there is a proven use for it, it's better to keep it disabled.
> 
>> +
>> 	/* set use of the 8-word descriptor and OSD foreground usage. */
>> 	if (priv->soc_info->use_extended_hwdesc)
>> 		cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
>> @@ -1321,6 +1328,25 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>> 	if (soc_info->has_osd)
>> 		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
>> +	if (soc_info->has_alpha)
>> +		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_ALPHAEN);
> 
> I remember you saying that OSD mode was not yet working on the JZ4780. So I can't see how you could have tested this.

Basically this is all stuff from the vendor kernel under the assumption that they know better than everyone of us.
On the other hand this whole patch is sort of optional and we know that the basic milestone to get HDMI working
is reached without it. So if you prefer we can drop it for the moment in v5 and leave it for further analysis later.

> 
>> +
>> +	/* Magic values from the vendor kernel for the priority thresholds. */
>> +	if (soc_info->has_pcfg)
>> +		regmap_write(priv->map, JZ_REG_LCD_PCFG,
>> +			     JZ_LCD_PCFG_PRI_MODE |
>> +			     JZ_LCD_PCFG_HP_BST_16 |
>> +			     (511 << JZ_LCD_PCFG_THRESHOLD2_OFFSET) |
>> +			     (400 << JZ_LCD_PCFG_THRESHOLD1_OFFSET) |
>> +			     (256 << JZ_LCD_PCFG_THRESHOLD0_OFFSET));
> 
> Unless you add a big comment that explains what these values do and why we do want them, I don't want magic values in here. The fact that the kernel vendor sets this doesn't mean it's needed and/or wanted.

Well, who has a contact within Ingenic?

> 
>> +
>> +	/* RGB output control may be superfluous. */
>> +	if (soc_info->has_rgbc)
>> +		regmap_write(priv->map, JZ_REG_LCD_RGBC,
>> +			     JZ_LCD_RGBC_RGB_FORMAT_ENABLE |
>> +			     JZ_LCD_RGBC_ODD_RGB |
>> +			     JZ_LCD_RGBC_EVEN_RGB);
> 
> ingenic-drm only supports RGB output right now, so I guess the RGB_FORMAT_ENABLE bit needs to be set in patch [2/10], otherwise patch [2/10] cannot state that it adds support for the JZ4780, if it doesn't actually work.
> 
> The other two bits can be dropped, they are already set in ingenic_drm_encoder_atomic_mode_set().

Ok.

> 
>> +
>> 	mutex_init(&priv->clk_mutex);
>> 	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
>> @@ -1484,6 +1510,9 @@ static const struct jz_soc_info jz4740_soc_info = {
>> 	.needs_dev_clk = true,
>> 	.has_osd = false,
>> 	.map_noncoherent = false,
>> +	.has_pcfg = false,
>> +	.has_recover = false,
>> +	.has_rgbc = false,
>> 	.max_width = 800,
>> 	.max_height = 600,
>> 	.formats_f1 = jz4740_formats,
>> @@ -1496,6 +1525,9 @@ static const struct jz_soc_info jz4725b_soc_info = {
>> 	.needs_dev_clk = false,
>> 	.has_osd = true,
>> 	.map_noncoherent = false,
>> +	.has_pcfg = false,
>> +	.has_recover = false,
>> +	.has_rgbc = false,
> 
> This is wrong, the JZ4725B and JZ4770 SoCs both have the RGBC register and the RECOVER bit.

Ok, good to know! Will change.

BR and thanks,
Nikolaus

> 
> Cheers,
> -Paul
> 
>> 	.max_width = 800,
>> 	.max_height = 600,
>> 	.formats_f1 = jz4725b_formats_f1,
>> @@ -1509,6 +1541,9 @@ static const struct jz_soc_info jz4770_soc_info = {
>> 	.needs_dev_clk = false,
>> 	.has_osd = true,
>> 	.map_noncoherent = true,
>> +	.has_pcfg = false,
>> +	.has_recover = false,
>> +	.has_rgbc = false,
>> 	.max_width = 1280,
>> 	.max_height = 720,
>> 	.formats_f1 = jz4770_formats_f1,
>> @@ -1521,6 +1556,10 @@ static const struct jz_soc_info jz4770_soc_info = {
>> static const struct jz_soc_info jz4780_soc_info = {
>> 	.needs_dev_clk = true,
>> 	.has_osd = true,
>> +	.has_alpha = true,
>> +	.has_pcfg = true,
>> +	.has_recover = true,
>> +	.has_rgbc = true,
>> 	.use_extended_hwdesc = true,
>> 	.max_width = 4096,
>> 	.max_height = 2048,
>> --
>> 2.31.1