[PATCH] drm/bridge: tc358767: Fix odd pixel alignment

[Marek Vasut]

On 10/28/24 2:52 PM, Maxime Ripard wrote:
> On Mon, Oct 28, 2024 at 01:36:58PM +0100, Marek Vasut wrote:
>> On 10/28/24 10:25 AM, Maxime Ripard wrote:
>>> On Sat, Oct 26, 2024 at 06:10:01AM +0200, Marek Vasut wrote:
>>>> Horizontal Timing Control0 Register 1/2 (HTIM01/HTIM02) Register
>>>> bitfields description state "These bits must be multiple of even
>>>> pixel". It is not possible to simply align every bitfield to the
>>>> nearest even pixel, because that would unalign the line width and
>>>> cause visible distortion. Instead, attempt to re-align the timings
>>>> such that the hardware requirement is fulfilled without changing
>>>> the line width if at all possible.
>>>>
>>>> Warn the user in case a panel with odd active pixel width or full
>>>> line width is used, this is not possible to support with this one
>>>> bridge.
>>>>
>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>> ---
>>>> Cc: Andrzej Hajda <andrzej.hajda at intel.com>
>>>> Cc: David Airlie <airlied at gmail.com>
>>>> Cc: Jernej Skrabec <jernej.skrabec at gmail.com>
>>>> Cc: Jonas Karlman <jonas at kwiboo.se>
>>>> Cc: Laurent Pinchart <Laurent.pinchart at ideasonboard.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>> Cc: Maxime Ripard <mripard at kernel.org>
>>>> Cc: Neil Armstrong <neil.armstrong at linaro.org>
>>>> Cc: Robert Foss <rfoss at kernel.org>
>>>> Cc: Simona Vetter <simona at ffwll.ch>
>>>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>>>> Cc: dri-devel at lists.freedesktop.org
>>>> ---
>>>>    drivers/gpu/drm/bridge/tc358767.c | 63 +++++++++++++++++++++++++++++--
>>>>    1 file changed, 60 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>>>> index 0a6894498267e..7968183510e63 100644
>>>> --- a/drivers/gpu/drm/bridge/tc358767.c
>>>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>>>> @@ -901,6 +901,63 @@ static int tc_set_common_video_mode(struct tc_data *tc,
>>>>    	int vsync_len = mode->vsync_end - mode->vsync_start;
>>>>    	int ret;
>>>> +	/*
>>>> +	 * Horizontal Timing Control0 Register 1/2 (HTIM01/HTIM02) Register
>>>> +	 * bitfields description state "These bits must be multiple of even
>>>> +	 * pixel". It is not possible to simply align every bitfield to the
>>>> +	 * nearest even pixel, because that would unalign the line width.
>>>> +	 * Instead, attempt to re-align the timings.
>>>> +	 */
>>>> +
>>>> +	/* Panels with odd active pixel count are not supported by the bridge */
>>>> +	if (mode->hdisplay & 1)
>>>> +		dev_warn(tc->dev, "Panels with odd pixel count per active line are not supported.\n");
>>>> +
>>>> +	/* HPW is odd */
>>>> +	if (hsync_len & 1) {
>>>> +		/* Make sure there is some margin left */
>>>> +		if (left_margin >= 2) {
>>>> +			/* Align HPW up */
>>>> +			hsync_len++;
>>>> +			left_margin--;
>>>> +		} else if (right_margin >= 2) {
>>>> +			/* Align HPW up */
>>>> +			hsync_len++;
>>>> +			right_margin--;
>>>> +		} else if (hsync_len > 2) {
>>>> +			/* Align HPW down as last-resort option */
>>>> +			hsync_len--;
>>>> +			left_margin++;
>>>> +		} else {
>>>> +			dev_warn(tc->dev, "HPW is odd, not enough margins to compensate.\n");
>>>> +		}
>>>> +	}
>>>> +
>>>> +	/* HBP is odd (HPW is surely even now) */
>>>> +	if (left_margin & 1) {
>>>> +		/* Make sure there is some margin left */
>>>> +		if (right_margin >= 2) {
>>>> +			/* Align HBP up */
>>>> +			left_margin++;
>>>> +			right_margin--;
>>>> +		} else if (hsync_len > 2) {
>>>> +			/* HPW is surely even and > 2, which means at least 4 */
>>>> +			hsync_len -= 2;
>>>> +			/*
>>>> +			 * Subtract 2 from sync pulse and distribute it between
>>>> +			 * margins. This aligns HBP and keeps HPW aligned.
>>>> +			 */
>>>> +			left_margin++;
>>>> +			right_margin++;
>>>> +		} else {
>>>> +			dev_warn(tc->dev, "HBP is odd, not enough pixels to compensate.\n");
>>>> +		}
>>>> +	}
>>>> +
>>>> +	/* HFP is odd (HBP and HPW is surely even now) */
>>>> +	if (right_margin & 1)
>>>> +		dev_warn(tc->dev, "HFP is odd, panels with odd pixel count per full line are not supported.\n");
>>>> +
>>>
>>> This should all happen in atomic_check, and reject modes that can't
>>> be supported.
> 
>> No, that would reject panels I need to support and which can be
>> supported by this bridge.
> 
> Then drop the warnings, either you support it or you don't.
These warnings are useful to notify users that something is not right. I 
find them useful when bringing up systems with this bridge.