diff mbox series

drm/bridge: tc358767: Fix odd pixel alignment

Message ID 20241026041019.247606-1-marex@denx.de (mailing list archive)
State New, archived
Headers show
Series drm/bridge: tc358767: Fix odd pixel alignment | expand

Commit Message

Marek Vasut Oct. 26, 2024, 4:10 a.m. UTC
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@denx.de>
---
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Robert Foss <rfoss@kernel.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/bridge/tc358767.c | 63 +++++++++++++++++++++++++++++--
 1 file changed, 60 insertions(+), 3 deletions(-)

Comments

Maxime Ripard Oct. 28, 2024, 9:25 a.m. UTC | #1
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@denx.de>
> ---
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@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.

Maxime
Marek Vasut Oct. 28, 2024, 12:36 p.m. UTC | #2
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@denx.de>
>> ---
>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>> Cc: Jonas Karlman <jonas@kwiboo.se>
>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Neil Armstrong <neil.armstrong@linaro.org>
>> Cc: Robert Foss <rfoss@kernel.org>
>> Cc: Simona Vetter <simona@ffwll.ch>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: dri-devel@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.
Maxime Ripard Oct. 28, 2024, 1:52 p.m. UTC | #3
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@denx.de>
> > > ---
> > > Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> > > Cc: David Airlie <airlied@gmail.com>
> > > Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > Cc: Jonas Karlman <jonas@kwiboo.se>
> > > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Maxime Ripard <mripard@kernel.org>
> > > Cc: Neil Armstrong <neil.armstrong@linaro.org>
> > > Cc: Robert Foss <rfoss@kernel.org>
> > > Cc: Simona Vetter <simona@ffwll.ch>
> > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > Cc: dri-devel@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.

Maxime
Maxime Ripard Oct. 28, 2024, 1:53 p.m. UTC | #4
On Mon, Oct 28, 2024 at 02:52:09PM +0100, 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@denx.de>
> > > > ---
> > > > Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> > > > Cc: David Airlie <airlied@gmail.com>
> > > > Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > > Cc: Jonas Karlman <jonas@kwiboo.se>
> > > > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Maxime Ripard <mripard@kernel.org>
> > > > Cc: Neil Armstrong <neil.armstrong@linaro.org>
> > > > Cc: Robert Foss <rfoss@kernel.org>
> > > > Cc: Simona Vetter <simona@ffwll.ch>
> > > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > > Cc: dri-devel@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.

Oh, and update the commit log, because so far it claims that you can't
support those panels.

Maxime
Marek Vasut Oct. 28, 2024, 2:49 p.m. UTC | #5
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@denx.de>
>>>> ---
>>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>>>> Cc: David Airlie <airlied@gmail.com>
>>>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>>>> Cc: Jonas Karlman <jonas@kwiboo.se>
>>>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>> Cc: Neil Armstrong <neil.armstrong@linaro.org>
>>>> Cc: Robert Foss <rfoss@kernel.org>
>>>> Cc: Simona Vetter <simona@ffwll.ch>
>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Cc: dri-devel@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.
Maxime Ripard Nov. 18, 2024, 3:19 p.m. UTC | #6
On Mon, Oct 28, 2024 at 03:49:42PM +0100, Marek Vasut wrote:
> 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@denx.de>
> > > > > ---
> > > > > Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> > > > > Cc: David Airlie <airlied@gmail.com>
> > > > > Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > > > Cc: Jonas Karlman <jonas@kwiboo.se>
> > > > > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > Cc: Maxime Ripard <mripard@kernel.org>
> > > > > Cc: Neil Armstrong <neil.armstrong@linaro.org>
> > > > > Cc: Robert Foss <rfoss@kernel.org>
> > > > > Cc: Simona Vetter <simona@ffwll.ch>
> > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > > > Cc: dri-devel@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.

Yeah, because you know what to do about them.

Unfortunately, you're not the only audience of those messages, and the
vast majority of the users will have no idea what these warnings are
about, and no way to fix them.

Maxime
Marek Vasut Nov. 18, 2024, 4:42 p.m. UTC | #7
On 11/18/24 4:19 PM, Maxime Ripard wrote:
> On Mon, Oct 28, 2024 at 03:49:42PM +0100, Marek Vasut wrote:
>> 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@denx.de>
>>>>>> ---
>>>>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>>>>>> Cc: David Airlie <airlied@gmail.com>
>>>>>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>>>>>> Cc: Jonas Karlman <jonas@kwiboo.se>
>>>>>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>>> Cc: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>> Cc: Robert Foss <rfoss@kernel.org>
>>>>>> Cc: Simona Vetter <simona@ffwll.ch>
>>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>> Cc: dri-devel@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.
> 
> Yeah, because you know what to do about them.
> 
> Unfortunately, you're not the only audience of those messages, and the
> vast majority of the users will have no idea what these warnings are
> about, and no way to fix them.
The net result of removing these warnings will be -- it will be harder 
for me to debug problems with this driver, and the rest of the audience 
will not be able to provide meaningful error reports because the 
warnings will not be present. That does not seem helpful.
diff mbox series

Patch

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");
+
 	dev_dbg(tc->dev, "set mode %dx%d\n",
 		mode->hdisplay, mode->vdisplay);
 	dev_dbg(tc->dev, "H margin %d,%d sync %d\n",
@@ -922,14 +979,14 @@  static int tc_set_common_video_mode(struct tc_data *tc,
 		return ret;
 
 	ret = regmap_write(tc->regmap, HTIM01,
-			   FIELD_PREP(HBPR, ALIGN(left_margin, 2)) |
-			   FIELD_PREP(HPW, ALIGN(hsync_len, 2)));
+			   FIELD_PREP(HBPR, left_margin) |
+			   FIELD_PREP(HPW, hsync_len));
 	if (ret)
 		return ret;
 
 	ret = regmap_write(tc->regmap, HTIM02,
 			   FIELD_PREP(HDISPR, ALIGN(mode->hdisplay, 2)) |
-			   FIELD_PREP(HFPR, ALIGN(right_margin, 2)));
+			   FIELD_PREP(HFPR, right_margin));
 	if (ret)
 		return ret;