Re: [PATCH] drm: mxsfb: Enable recovery on underflow

From: Marek Vasut <marex@denx.de>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: ch@denx.de, Robert Foss <robert.foss@linaro.org>,
	Emil Velikov <emil.l.velikov@gmail.com>,
	Daniel Abrecht <public@danielabrecht.ch>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm: mxsfb: Enable recovery on underflow
Date: Thu, 1 Jul 2021 00:49:02 +0200	[thread overview]
Message-ID: <6326d213-548c-0c80-5e35-cd7a76dacade@denx.de> (raw)
In-Reply-To: <YNCCgCWA8KdkS/df@pendragon.ideasonboard.com>

On 6/21/21 2:13 PM, Laurent Pinchart wrote:
> Hi Marek,
> 
> Thank you for the patch.
> 
> On Mon, Jun 21, 2021 at 12:47:01AM +0200, Marek Vasut wrote:
>> There is some sort of corner case behavior of the controller,
>> which could rarely be triggered at least on i.MX6SX connected
>> to 800x480 DPI panel and i.MX8MM connected to DPI->DSI->LVDS
>> bridged 1920x1080 panel (and likely on other setups too), where
>> the image on the panel shifts to the right and wraps around.
>> This happens either when the controller is enabled on boot or
>> even later during run time. The condition does not correct
>> itself automatically, i.e. the display image remains shifted.
>>
>> It seems this problem is known and is due to sporadic underflows
>> of the LCDIF FIFO. While the LCDIF IP does have underflow/overflow
>> IRQs, neither of the IRQs trigger and neither IRQ status bit is
>> asserted when this condition occurs.
>>
>> All known revisions of the LCDIF IP have CTRL1 RECOVER_ON_UNDERFLOW
>> bit, which is described in the reference manual since i.MX23 as
>> "
>>    Set this bit to enable the LCDIF block to recover in the next
>>    field/frame if there was an underflow in the current field/frame.
>> "
>> Enable this bit to mitigate the sporadic underflows.
>>
>> Fixes: 45d59d704080 ("drm: Add new driver for MXSFB controller")
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Daniel Abrecht <public@danielabrecht.ch>
>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: Stefan Agner <stefan@agner.ch>
>> ---
>>   drivers/gpu/drm/mxsfb/mxsfb_kms.c  | 29 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/mxsfb/mxsfb_regs.h |  1 +
>>   2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>> index 300e7bab0f43..01e0f525360f 100644
>> --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>> +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c
>> @@ -115,6 +115,35 @@ static void mxsfb_enable_controller(struct mxsfb_drm_private *mxsfb)
>>   	reg |= VDCTRL4_SYNC_SIGNALS_ON;
>>   	writel(reg, mxsfb->base + LCDC_VDCTRL4);
>>   
>> +	/*
>> +	 * Enable recovery on underflow.
>> +	 *
>> +	 * There is some sort of corner case behavior of the controller,
>> +	 * which could rarely be triggered at least on i.MX6SX connected
>> +	 * to 800x480 DPI panel and i.MX8MM connected to DPI->DSI->LVDS
>> +	 * bridged 1920x1080 panel (and likely on other setups too), where
>> +	 * the image on the panel shifts to the right and wraps around.
>> +	 * This happens either when the controller is enabled on boot or
>> +	 * even later during run time. The condition does not correct
>> +	 * itself automatically, i.e. the display image remains shifted.
>> +	 *
>> +	 * It seems this problem is known and is due to sporadic underflows
>> +	 * of the LCDIF FIFO. While the LCDIF IP does have underflow/overflow
>> +	 * IRQs, neither of the IRQs trigger and neither IRQ status bit is
>> +	 * asserted when this condition occurs.
>> +	 *
>> +	 * All known revisions of the LCDIF IP have CTRL1 RECOVER_ON_UNDERFLOW
>> +	 * bit, which is described in the reference manual since i.MX23 as
>> +	 * "
>> +	 *   Set this bit to enable the LCDIF block to recover in the next
>> +	 *   field/frame if there was an underflow in the current field/frame.
>> +	 * "
>> +	 * Enable this bit to mitigate the sporadic underflows.
>> +	 */
>> +	reg = readl(mxsfb->base + LCDC_CTRL1);
>> +	reg |= CTRL1_RECOVER_ON_UNDERFLOW;
>> +	writel(reg, mxsfb->base + LCDC_CTRL1);
> 
> Looks good to me. Thanks for the detailed explanation.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

So who do I CC to pick it? Robert ? There are a few more mxsfb fixes 
which are RB'd and would be nice if they were picked too.