Nouveau - Sep 2022 - [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes

Hi,

On Wed, Aug 31, 2022 at 03:44:52AM +0200, Mateusz Kwiatkowski
wrote:
> > +#define NTSC_HFP_DURATION_TYP_NS?? ?1500
> > +#define NTSC_HFP_DURATION_MIN_NS?? ?1270
> > +#define NTSC_HFP_DURATION_MAX_NS?? ?2220
> 
> You've defined those min/typ/max ranges, but you're not using the
"typ" field
> for anything other than hslen.
Yeah... I've left most of them because it was so hard to find most of
them, it's useful at least for documentation purposes. And it's a define
so there's pretty much no downside to it as far as the final binary is
involved.
> The actual "typical" value is thus always the midpoint, which
isn't
> necessarily the best choice.
> 
> In particular, for the standard 720px wide modes at 13.5 MHz, hsync_start
> ends up being 735 for 480i and 734 for 576i, instead of 736 and 732
requested
> by BT.601. That's all obviously within tolerances, but the image ends
up
> noticeably off-center (at least on modern TVs), especially in the 576i
case.
I'll try to fix that up.
> > +?? ?htotal = params->line_duration_ns * pixel_clock_hz /
NSEC_PER_SEC;
> 
> You're multiplying an unsigned int and an unsigned long - both types
are only
> required to be 32 bit, so this is likely to overflow. You need to use a
cast to
> unsigned long long, and then call do_div() for 64-bit division.
> 
> This actually overflowed on me on my Pi running ARM32 kernel, resulting in
> negative horizontal porch lengths, and drm_helper_probe_add_cmdline_mode()
> taking over the mode generation (badly), and a horrible mess on screen.
Indeed, that's bad.
> > +?? ?vfp = vfp_min + (porches_rem / 2);
> > +?? ?vbp = porches - vfp;
> 
> Relative position of the vertical sync within the VBI effectively moves the
> image up and down. Adding that (porches_rem / 2) moves the image up off
center
> by that many pixels. I'd keep the VFP always at minimum to keep the
image
> centered.
And you would increase the back porch only then?

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20220905/e9dd4477/attachment.sig>

Hi Maxime,

W dniu 5.09.2022 o 15:37, Maxime Ripard pisze:
>>> +??? vfp = vfp_min + (porches_rem / 2);
>>> +??? vbp = porches - vfp;
>>
>> Relative position of the vertical sync within the VBI effectively moves
the
>> image up and down. Adding that (porches_rem / 2) moves the image up off
center
>> by that many pixels. I'd keep the VFP always at minimum to keep the
image
>> centered.
>
> And you would increase the back porch only then?
Well, increasing vbp only gives a centered image with the default 480i/576i
resolutions. However, only ever changing vbp will cause the image to be always
at the bottom of the screen when the active line count is decreased (e.g.
setting the resolution to 720x480 but for 50Hz "PAL" - like many game
consoles
did back in the day).

I believe that the perfect solution would:

- Use the canonical / standard-defined blanking line counts for the standard
? vertical resolutions (480/486/576)
- Increase vfp and vbp from there by the same number if a smaller number of
? active lines is specified, so that the resulting image is centered
- Likewise, decrease vfp and vbp by the same number if the active line number
? is larger and there is still leeway (this should allow for seamless handling
? of 480i vs. 486i for 60 Hz "NTSC")
- If even more active lines are specified, once the limit for vfp is hit, then
? decrease vbp only - the resulting image will definitely be off-center, but
? there's no other way

I hope this makes sense for you as well.

Best regards,
Mateusz Kwiatkowski