Re: [PATCH] drm: lcdif: Set and enable FIFO Panic threshold

[Marek Vasut <marex@denx.de>]

On 10/27/22 07:45, Liu Ying wrote:

Hi,

[...]

>> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
>> index a5302006c02cd..aee7babb5fa5c 100644
>> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
>> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
>> @@ -341,6 +341,18 @@ static void lcdif_enable_controller(struct lcdif_drm_private *lcdif)
>>   	reg = readl(lcdif->base + LCDC_V8_CTRLDESCL0_5);
>>   	reg |= CTRLDESCL0_5_EN;
>>   	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
>> +
>> +	/* Set FIFO Panic watermarks, low 1/3, high 2/3 . */
>> +	writel(FIELD_PREP(PANIC0_THRES_LOW_MASK, 1 * PANIC0_THRES_RANGE / 3) |
>> +	       FIELD_PREP(PANIC0_THRES_HIGH_MASK, 2 * PANIC0_THRES_RANGE / 3),
> 
> Better to define PANIC0_THRES_{LOW,HIGH}(n) macros in lcdif_regs.h?
> 
> Downstream kernel uses the below threshold values:
> a) i.MX8mp EVK board with LPDDR4
> 1/3 and 2/3 for LCDIF{1,2} + DSI/LVDS - default values in driver
> 1/2 and 3/4 for LCDIF3 + HDMI - set in device tree
> 
> b) i.MX8mp EVK board with DDR4
> 1/3 and 2/3 for LCDIF{1,2} + DSI/LVDS - default values in driver
> 2/3 and 3/3 for LCDIF3 + HDMI - set in devic tree
> 
> Jian told me that LCDIF3 needs different sets of threshold values for
> different types of DDR to avoid 4k HDMI display issues and the
> threshold values impact overall DDR/bus utilization(?), so downstream
> kernel chooses to get optional threshold value properties from LCDIF DT
> node.
> 
> Instead of always using 1/3 and 2/3, maybe there are three options:
> 1) Same to downstream kernel, take 1/3 and 2/3 as default values and
> get optional threshold values from DT properties - no additional
> properties are acceptable in the existing DT binding doc?
> 2) Check pixel clock rate, and if it is greater than a certain value,
> use 2/3 and 3/3.  Otherwise, use 1/3 and 2/3.
> 3) Always use 2/3 and 3/3.

Why 2/3 and 3/3 instead of 1/3 and 2/3 ?

Seems like 1/3 and 2/3 provides enough FIFO margin for every scenario.

>> +	       lcdif->base + LCDC_V8_PANIC0_THRES);
>> +
>> +	/*
>> +	 * Enable FIFO Panic, this does not generate interrupt, but
>> +	 * boosts NoC priority based on FIFO Panic watermarks.
>> +	 */
>> +	writel(INT_ENABLE_D1_PLANE_PANIC_EN,
>> +	       lcdif->base + LCDC_V8_INT_ENABLE_D1);
> 
> This should be enabled _before_ LCDIF controller starts to fetch
> pixels, otherwise, there is chance that the FIFO still underflows.

That means _before_ DISP_PARA_DISP_ON or CTRLDESCL0_5_EN ?

>>   }
>>   
>>   static void lcdif_disable_controller(struct lcdif_drm_private *lcdif)
>> @@ -348,6 +360,9 @@ static void lcdif_disable_controller(struct lcdif_drm_private *lcdif)
>>   	u32 reg;
>>   	int ret;
>>   
>> +	/* Disable FIFO Panic NoC priority booster. */
>> +	writel(0, lcdif->base + LCDC_V8_INT_ENABLE_D1);
> 
> Similar to enablement, this should be disabled _after_ LCDIF controller
> stops fetching pixels.

Same question as above applies, which bits controls that part, DISP_ON 
or CTRLDESCL0_5_EN ? I suspect the later.

>> +
>>   	reg = readl(lcdif->base + LCDC_V8_CTRLDESCL0_5);
>>   	reg &= ~CTRLDESCL0_5_EN;
>>   	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
>> diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h
>> index fb74eb5ccbf1d..3d2f81d6f995e 100644
>> --- a/drivers/gpu/drm/mxsfb/lcdif_regs.h
>> +++ b/drivers/gpu/drm/mxsfb/lcdif_regs.h
>> @@ -255,6 +255,7 @@
>>   
>>   #define PANIC0_THRES_LOW_MASK		GENMASK(24, 16)
>>   #define PANIC0_THRES_HIGH_MASK		GENMASK(8, 0)
>> +#define PANIC0_THRES_RANGE		512
> 
> Should be 511? If high threshold is 3/3 and PANIC0_THRES_RANGE = 512,
> PANIC0_THRES_HIGH will overflow and zero is set.

Let's rename this to PANIC0_THRES_MAX too.