Re: [Intel-gfx] [PATCH v4 11/30] drm/modes: Add a function to generate analog display modes
Hi, On Sat, Oct 15, 2022 at 05:04:50PM +0200, Noralf Trønnes wrote: > Den 13.10.2022 10.36, skrev Maxime Ripard: > > On Sat, Oct 01, 2022 at 02:52:06PM +0200, Noralf Trønnes wrote: > >> Den 29.09.2022 18.31, skrev Maxime Ripard: > >>> Multiple drivers (meson, vc4, sun4i) define analog TV 525-lines and > >>> 625-lines modes in their drivers. > >>> > >>> Since those modes are fairly standard, and that we'll need to use them > >>> in more places in the future, it makes sense to move their definition > >>> into the core framework. > >>> > >>> However, analog display usually have fairly loose timings requirements, > >>> the only discrete parameters being the total number of lines and pixel > >>> clock frequency. Thus, we created a function that will create a display > >>> mode from the standard, the pixel frequency and the active area. > >>> > >>> Signed-off-by: Maxime Ripard > >>> > >>> --- > >>> > >>> Changes in v4: > >>> - Reworded the line length check comment > >>> - Switch to HZ_PER_KHZ in tests > >>> - Use previous timing to fill our mode > >>> - Move the number of lines check earlier > >>> --- > >>> drivers/gpu/drm/drm_modes.c| 474 > >>> + > >>> drivers/gpu/drm/tests/Makefile | 1 + > >>> drivers/gpu/drm/tests/drm_modes_test.c | 144 ++ > >>> include/drm/drm_modes.h| 17 ++ > >>> 4 files changed, 636 insertions(+) > >>> > >> > >> I haven't followed the discussion on this patch, but it seems rather > >> excessive to add over 600 lines of code (including tests) to add 2 fixed > >> modes. And it's very difficult to see from the code what the actual > >> display mode timings really are, which would be useful for other > >> developers down the road wanting to use them. > >> > >> Why not just hardcode the modes? > > > > Yeah, I have kind of the same feeling tbh, but it was asked back on the > > v1 to ease the transition of old fbdev drivers, since they will need > > such a function: > > https://lore.kernel.org/dri-devel/CAMuHMdUrwzPYjA0wdR7ADj5Ov6+m03JbnY8fBYzRYyWDuNm5=g...@mail.gmail.com/ > > > > If that's the case I suggest you just hardcode them for now and leave > the complexity to the developer doing the actual conversion of the fbdev > driver. Who knows when that will happen, but that person will have your > well documented and discussed work to fall back on. I'd rather not, tbh. We've collectively spent weeks figuring this out, reviewing it and so on, I very much want to avoid doing this all over again if it's going to be useful at some point. Jani also wanted to expose a function and not a raw mode, so this patch also addresses that: https://lore.kernel.org/dri-devel/8735eeg31e@intel.com/ Maxime
Re: [Intel-gfx] [PATCH v4 11/30] drm/modes: Add a function to generate analog display modes
Den 13.10.2022 10.36, skrev Maxime Ripard: > Hi Noralf, > > On Sat, Oct 01, 2022 at 02:52:06PM +0200, Noralf Trønnes wrote: >> Den 29.09.2022 18.31, skrev Maxime Ripard: >>> Multiple drivers (meson, vc4, sun4i) define analog TV 525-lines and >>> 625-lines modes in their drivers. >>> >>> Since those modes are fairly standard, and that we'll need to use them >>> in more places in the future, it makes sense to move their definition >>> into the core framework. >>> >>> However, analog display usually have fairly loose timings requirements, >>> the only discrete parameters being the total number of lines and pixel >>> clock frequency. Thus, we created a function that will create a display >>> mode from the standard, the pixel frequency and the active area. >>> >>> Signed-off-by: Maxime Ripard >>> >>> --- >>> >>> Changes in v4: >>> - Reworded the line length check comment >>> - Switch to HZ_PER_KHZ in tests >>> - Use previous timing to fill our mode >>> - Move the number of lines check earlier >>> --- >>> drivers/gpu/drm/drm_modes.c| 474 >>> + >>> drivers/gpu/drm/tests/Makefile | 1 + >>> drivers/gpu/drm/tests/drm_modes_test.c | 144 ++ >>> include/drm/drm_modes.h| 17 ++ >>> 4 files changed, 636 insertions(+) >>> >> >> I haven't followed the discussion on this patch, but it seems rather >> excessive to add over 600 lines of code (including tests) to add 2 fixed >> modes. And it's very difficult to see from the code what the actual >> display mode timings really are, which would be useful for other >> developers down the road wanting to use them. >> >> Why not just hardcode the modes? > > Yeah, I have kind of the same feeling tbh, but it was asked back on the > v1 to ease the transition of old fbdev drivers, since they will need > such a function: > https://lore.kernel.org/dri-devel/CAMuHMdUrwzPYjA0wdR7ADj5Ov6+m03JbnY8fBYzRYyWDuNm5=g...@mail.gmail.com/ > If that's the case I suggest you just hardcode them for now and leave the complexity to the developer doing the actual conversion of the fbdev driver. Who knows when that will happen, but that person will have your well documented and discussed work to fall back on. Noralf.
Re: [Intel-gfx] [PATCH v4 11/30] drm/modes: Add a function to generate analog display modes
Hi Noralf, On Sat, Oct 01, 2022 at 02:52:06PM +0200, Noralf Trønnes wrote: > Den 29.09.2022 18.31, skrev Maxime Ripard: > > Multiple drivers (meson, vc4, sun4i) define analog TV 525-lines and > > 625-lines modes in their drivers. > > > > Since those modes are fairly standard, and that we'll need to use them > > in more places in the future, it makes sense to move their definition > > into the core framework. > > > > However, analog display usually have fairly loose timings requirements, > > the only discrete parameters being the total number of lines and pixel > > clock frequency. Thus, we created a function that will create a display > > mode from the standard, the pixel frequency and the active area. > > > > Signed-off-by: Maxime Ripard > > > > --- > > > > Changes in v4: > > - Reworded the line length check comment > > - Switch to HZ_PER_KHZ in tests > > - Use previous timing to fill our mode > > - Move the number of lines check earlier > > --- > > drivers/gpu/drm/drm_modes.c| 474 > > + > > drivers/gpu/drm/tests/Makefile | 1 + > > drivers/gpu/drm/tests/drm_modes_test.c | 144 ++ > > include/drm/drm_modes.h| 17 ++ > > 4 files changed, 636 insertions(+) > > > > I haven't followed the discussion on this patch, but it seems rather > excessive to add over 600 lines of code (including tests) to add 2 fixed > modes. And it's very difficult to see from the code what the actual > display mode timings really are, which would be useful for other > developers down the road wanting to use them. > > Why not just hardcode the modes? Yeah, I have kind of the same feeling tbh, but it was asked back on the v1 to ease the transition of old fbdev drivers, since they will need such a function: https://lore.kernel.org/dri-devel/CAMuHMdUrwzPYjA0wdR7ADj5Ov6+m03JbnY8fBYzRYyWDuNm5=g...@mail.gmail.com/ Maxime signature.asc Description: PGP signature
Re: [Intel-gfx] [PATCH v4 11/30] drm/modes: Add a function to generate analog display modes
Den 29.09.2022 18.31, skrev Maxime Ripard:
> Multiple drivers (meson, vc4, sun4i) define analog TV 525-lines and
> 625-lines modes in their drivers.
>
> Since those modes are fairly standard, and that we'll need to use them
> in more places in the future, it makes sense to move their definition
> into the core framework.
>
> However, analog display usually have fairly loose timings requirements,
> the only discrete parameters being the total number of lines and pixel
> clock frequency. Thus, we created a function that will create a display
> mode from the standard, the pixel frequency and the active area.
>
> Signed-off-by: Maxime Ripard
>
> ---
>
> Changes in v4:
> - Reworded the line length check comment
> - Switch to HZ_PER_KHZ in tests
> - Use previous timing to fill our mode
> - Move the number of lines check earlier
> ---
> drivers/gpu/drm/drm_modes.c| 474
> +
> drivers/gpu/drm/tests/Makefile | 1 +
> drivers/gpu/drm/tests/drm_modes_test.c | 144 ++
> include/drm/drm_modes.h| 17 ++
> 4 files changed, 636 insertions(+)
>
I haven't followed the discussion on this patch, but it seems rather
excessive to add over 600 lines of code (including tests) to add 2 fixed
modes. And it's very difficult to see from the code what the actual
display mode timings really are, which would be useful for other
developers down the road wanting to use them.
Why not just hardcode the modes?
Noralf.
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 304004fb80aa..7cf2fe98d7d2 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -116,6 +116,480 @@ void drm_mode_probed_add(struct drm_connector
> *connector,
> }
> EXPORT_SYMBOL(drm_mode_probed_add);
>
> +enum drm_mode_analog {
> + DRM_MODE_ANALOG_NTSC, /* 525 lines, 60Hz */
> + DRM_MODE_ANALOG_PAL, /* 625 lines, 50Hz */
> +};
> +
> +/*
> + * The timings come from:
> + * -
> https://web.archive.org/web/20220406232708/http://www.kolumbus.fi/pami1/video/pal_ntsc.html
> + * -
> https://web.archive.org/web/20220406124914/http://martin.hinner.info/vga/pal.html
> + * -
> https://web.archive.org/web/20220609202433/http://www.batsocks.co.uk/readme/video_timing.htm
> + */
> +#define NTSC_LINE_DURATION_NS63556U
> +#define NTSC_LINES_NUMBER525
> +
> +#define NTSC_HBLK_DURATION_TYP_NS10900U
> +#define NTSC_HBLK_DURATION_MIN_NS(NTSC_HBLK_DURATION_TYP_NS - 200)
> +#define NTSC_HBLK_DURATION_MAX_NS(NTSC_HBLK_DURATION_TYP_NS + 200)
> +
> +#define NTSC_HACT_DURATION_TYP_NS(NTSC_LINE_DURATION_NS -
> NTSC_HBLK_DURATION_TYP_NS)
> +#define NTSC_HACT_DURATION_MIN_NS(NTSC_LINE_DURATION_NS -
> NTSC_HBLK_DURATION_MAX_NS)
> +#define NTSC_HACT_DURATION_MAX_NS(NTSC_LINE_DURATION_NS -
> NTSC_HBLK_DURATION_MIN_NS)
> +
> +#define NTSC_HFP_DURATION_TYP_NS 1500
> +#define NTSC_HFP_DURATION_MIN_NS 1270
> +#define NTSC_HFP_DURATION_MAX_NS 2220
> +
> +#define NTSC_HSLEN_DURATION_TYP_NS 4700
> +#define NTSC_HSLEN_DURATION_MIN_NS (NTSC_HSLEN_DURATION_TYP_NS - 100)
> +#define NTSC_HSLEN_DURATION_MAX_NS (NTSC_HSLEN_DURATION_TYP_NS + 100)
> +
> +#define NTSC_HBP_DURATION_TYP_NS 4700
> +
> +/*
> + * I couldn't find the actual tolerance for the back porch, so let's
> + * just reuse the sync length ones.
> + */
> +#define NTSC_HBP_DURATION_MIN_NS (NTSC_HBP_DURATION_TYP_NS - 100)
> +#define NTSC_HBP_DURATION_MAX_NS (NTSC_HBP_DURATION_TYP_NS + 100)
> +
> +#define PAL_LINE_DURATION_NS 64000U
> +#define PAL_LINES_NUMBER 625
> +
> +#define PAL_HACT_DURATION_TYP_NS 51950U
> +#define PAL_HACT_DURATION_MIN_NS (PAL_HACT_DURATION_TYP_NS - 100)
> +#define PAL_HACT_DURATION_MAX_NS (PAL_HACT_DURATION_TYP_NS + 400)
> +
> +#define PAL_HBLK_DURATION_TYP_NS (PAL_LINE_DURATION_NS -
> PAL_HACT_DURATION_TYP_NS)
> +#define PAL_HBLK_DURATION_MIN_NS (PAL_LINE_DURATION_NS -
> PAL_HACT_DURATION_MAX_NS)
> +#define PAL_HBLK_DURATION_MAX_NS (PAL_LINE_DURATION_NS -
> PAL_HACT_DURATION_MIN_NS)
> +
> +#define PAL_HFP_DURATION_TYP_NS 1650
> +#define PAL_HFP_DURATION_MIN_NS (PAL_HFP_DURATION_TYP_NS - 100)
> +#define PAL_HFP_DURATION_MAX_NS (PAL_HFP_DURATION_TYP_NS + 400)
> +
> +#define PAL_HSLEN_DURATION_TYP_NS4700
> +#define PAL_HSLEN_DURATION_MIN_NS(PAL_HSLEN_DURATION_TYP_NS - 200)
> +#define PAL_HSLEN_DURATION_MAX_NS(PAL_HSLEN_DURATION_TYP_NS + 200)
> +
> +#define PAL_HBP_DURATION_TYP_NS 5700
> +#define PAL_HBP_DURATION_MIN_NS (PAL_HBP_DURATION_TYP_NS - 200)
> +#define PAL_HBP_DURATION_MAX_NS (PAL_HBP_DURATION_TYP_NS + 200)
> +
> +struct analog_param_field {
> + unsigned int even, odd;
> +};
> +
> +#define PARAM_FIELD(_odd, _even) \
> + { .even = _even, .odd = _odd }
> +
> +struct analog_param_range {
> + unsigned intmin, typ, max;
>
[Intel-gfx] [PATCH v4 11/30] drm/modes: Add a function to generate analog display modes
Multiple drivers (meson, vc4, sun4i) define analog TV 525-lines and
625-lines modes in their drivers.
Since those modes are fairly standard, and that we'll need to use them
in more places in the future, it makes sense to move their definition
into the core framework.
However, analog display usually have fairly loose timings requirements,
the only discrete parameters being the total number of lines and pixel
clock frequency. Thus, we created a function that will create a display
mode from the standard, the pixel frequency and the active area.
Signed-off-by: Maxime Ripard
---
Changes in v4:
- Reworded the line length check comment
- Switch to HZ_PER_KHZ in tests
- Use previous timing to fill our mode
- Move the number of lines check earlier
---
drivers/gpu/drm/drm_modes.c| 474 +
drivers/gpu/drm/tests/Makefile | 1 +
drivers/gpu/drm/tests/drm_modes_test.c | 144 ++
include/drm/drm_modes.h| 17 ++
4 files changed, 636 insertions(+)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 304004fb80aa..7cf2fe98d7d2 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -116,6 +116,480 @@ void drm_mode_probed_add(struct drm_connector *connector,
}
EXPORT_SYMBOL(drm_mode_probed_add);
+enum drm_mode_analog {
+ DRM_MODE_ANALOG_NTSC, /* 525 lines, 60Hz */
+ DRM_MODE_ANALOG_PAL, /* 625 lines, 50Hz */
+};
+
+/*
+ * The timings come from:
+ * -
https://web.archive.org/web/20220406232708/http://www.kolumbus.fi/pami1/video/pal_ntsc.html
+ * -
https://web.archive.org/web/20220406124914/http://martin.hinner.info/vga/pal.html
+ * -
https://web.archive.org/web/20220609202433/http://www.batsocks.co.uk/readme/video_timing.htm
+ */
+#define NTSC_LINE_DURATION_NS 63556U
+#define NTSC_LINES_NUMBER 525
+
+#define NTSC_HBLK_DURATION_TYP_NS 10900U
+#define NTSC_HBLK_DURATION_MIN_NS (NTSC_HBLK_DURATION_TYP_NS - 200)
+#define NTSC_HBLK_DURATION_MAX_NS (NTSC_HBLK_DURATION_TYP_NS + 200)
+
+#define NTSC_HACT_DURATION_TYP_NS (NTSC_LINE_DURATION_NS -
NTSC_HBLK_DURATION_TYP_NS)
+#define NTSC_HACT_DURATION_MIN_NS (NTSC_LINE_DURATION_NS -
NTSC_HBLK_DURATION_MAX_NS)
+#define NTSC_HACT_DURATION_MAX_NS (NTSC_LINE_DURATION_NS -
NTSC_HBLK_DURATION_MIN_NS)
+
+#define NTSC_HFP_DURATION_TYP_NS 1500
+#define NTSC_HFP_DURATION_MIN_NS 1270
+#define NTSC_HFP_DURATION_MAX_NS 2220
+
+#define NTSC_HSLEN_DURATION_TYP_NS 4700
+#define NTSC_HSLEN_DURATION_MIN_NS (NTSC_HSLEN_DURATION_TYP_NS - 100)
+#define NTSC_HSLEN_DURATION_MAX_NS (NTSC_HSLEN_DURATION_TYP_NS + 100)
+
+#define NTSC_HBP_DURATION_TYP_NS 4700
+
+/*
+ * I couldn't find the actual tolerance for the back porch, so let's
+ * just reuse the sync length ones.
+ */
+#define NTSC_HBP_DURATION_MIN_NS (NTSC_HBP_DURATION_TYP_NS - 100)
+#define NTSC_HBP_DURATION_MAX_NS (NTSC_HBP_DURATION_TYP_NS + 100)
+
+#define PAL_LINE_DURATION_NS 64000U
+#define PAL_LINES_NUMBER 625
+
+#define PAL_HACT_DURATION_TYP_NS 51950U
+#define PAL_HACT_DURATION_MIN_NS (PAL_HACT_DURATION_TYP_NS - 100)
+#define PAL_HACT_DURATION_MAX_NS (PAL_HACT_DURATION_TYP_NS + 400)
+
+#define PAL_HBLK_DURATION_TYP_NS (PAL_LINE_DURATION_NS -
PAL_HACT_DURATION_TYP_NS)
+#define PAL_HBLK_DURATION_MIN_NS (PAL_LINE_DURATION_NS -
PAL_HACT_DURATION_MAX_NS)
+#define PAL_HBLK_DURATION_MAX_NS (PAL_LINE_DURATION_NS -
PAL_HACT_DURATION_MIN_NS)
+
+#define PAL_HFP_DURATION_TYP_NS1650
+#define PAL_HFP_DURATION_MIN_NS(PAL_HFP_DURATION_TYP_NS - 100)
+#define PAL_HFP_DURATION_MAX_NS(PAL_HFP_DURATION_TYP_NS + 400)
+
+#define PAL_HSLEN_DURATION_TYP_NS 4700
+#define PAL_HSLEN_DURATION_MIN_NS (PAL_HSLEN_DURATION_TYP_NS - 200)
+#define PAL_HSLEN_DURATION_MAX_NS (PAL_HSLEN_DURATION_TYP_NS + 200)
+
+#define PAL_HBP_DURATION_TYP_NS5700
+#define PAL_HBP_DURATION_MIN_NS(PAL_HBP_DURATION_TYP_NS - 200)
+#define PAL_HBP_DURATION_MAX_NS(PAL_HBP_DURATION_TYP_NS + 200)
+
+struct analog_param_field {
+ unsigned int even, odd;
+};
+
+#define PARAM_FIELD(_odd, _even) \
+ { .even = _even, .odd = _odd }
+
+struct analog_param_range {
+ unsigned intmin, typ, max;
+};
+
+#define PARAM_RANGE(_min, _typ, _max) \
+ { .min = _min, .typ = _typ, .max = _max }
+
+struct analog_parameters {
+ unsigned intnum_lines;
+ unsigned intline_duration_ns;
+
+ struct analog_param_range hact_ns;
+ struct analog_param_range hfp_ns;
+ struct analog_param_range hslen_ns;
+ struct analog_param_range hbp_ns;
+ struct analog_param_range hblk_ns;
+
+ unsigned intbt601_hfp;
+
+ struct analog_param_field
