Re: [PATCH v1 04/35] drm/modes: Introduce 480i and 576i modes

From: Thomas Zimmermann
Date: Tue Aug 02 2022 - 10:16:36 EST


Hi

Am 02.08.22 um 15:58 schrieb Jani Nikula:
On Fri, 29 Jul 2022, Maxime Ripard <maxime@xxxxxxxxxx> wrote:
Multiple drivers (meson, vc4) define the analog TV 525-lines and 625-lines
modes in the drivers.

Since those modes are fairly standards, and that we'll need to use them in
more places in the future, let's move the meson definition into the
framework.

I think you should always expose interfaces, not data. Data is not an
interface, and I think this sets a bad example that will be cargo
culted.

Although I wrote the opposite wrt patch 8, I agree with Jani here when it comes to 'official' interfaces. The cases I've seen of exported data structures often lead to intransparent code.

Best regards
Thomas



BR,
Jani.


The meson one was chosen because vc4's isn't accurate and doesn't amount to
525 and 625 lines.

Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 304004fb80aa..a4c1bd688338 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -48,6 +48,24 @@
#include "drm_crtc_internal.h"
+const struct drm_display_mode drm_mode_480i = {
+ DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 13500,
+ 720, 739, 801, 858, 0,
+ 480, 488, 494, 525, 0,
+ DRM_MODE_FLAG_INTERLACE),
+ .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3,
+};
+EXPORT_SYMBOL_GPL(drm_mode_480i);
+
+const struct drm_display_mode drm_mode_576i = {
+ DRM_MODE("720x576i", DRM_MODE_TYPE_DRIVER, 13500,
+ 720, 732, 795, 864, 0,
+ 576, 580, 586, 625, 0,
+ DRM_MODE_FLAG_INTERLACE),
+ .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3,
+};
+EXPORT_SYMBOL_GPL(drm_mode_576i);
+
/**
* drm_mode_debug_printmodeline - print a mode to dmesg
* @mode: mode to print
diff --git a/drivers/gpu/drm/meson/meson_encoder_cvbs.c b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
index 8110a6e39320..98ec3e563155 100644
--- a/drivers/gpu/drm/meson/meson_encoder_cvbs.c
+++ b/drivers/gpu/drm/meson/meson_encoder_cvbs.c
@@ -45,21 +45,11 @@ struct meson_encoder_cvbs {
struct meson_cvbs_mode meson_cvbs_modes[MESON_CVBS_MODES_COUNT] = {
{ /* PAL */
.enci = &meson_cvbs_enci_pal,
- .mode = {
- DRM_MODE("720x576i", DRM_MODE_TYPE_DRIVER, 13500,
- 720, 732, 795, 864, 0, 576, 580, 586, 625, 0,
- DRM_MODE_FLAG_INTERLACE),
- .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3,
- },
+ .mode = &drm_mode_576i,
},
{ /* NTSC */
.enci = &meson_cvbs_enci_ntsc,
- .mode = {
- DRM_MODE("720x480i", DRM_MODE_TYPE_DRIVER, 13500,
- 720, 739, 801, 858, 0, 480, 488, 494, 525, 0,
- DRM_MODE_FLAG_INTERLACE),
- .picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3,
- },
+ .mode = &drm_mode_480i,
},
};
@@ -71,7 +61,7 @@ meson_cvbs_get_mode(const struct drm_display_mode *req_mode)
for (i = 0; i < MESON_CVBS_MODES_COUNT; ++i) {
struct meson_cvbs_mode *meson_mode = &meson_cvbs_modes[i];
- if (drm_mode_match(req_mode, &meson_mode->mode,
+ if (drm_mode_match(req_mode, meson_mode->mode,
DRM_MODE_MATCH_TIMINGS |
DRM_MODE_MATCH_CLOCK |
DRM_MODE_MATCH_FLAGS |
@@ -104,7 +94,7 @@ static int meson_encoder_cvbs_get_modes(struct drm_bridge *bridge,
for (i = 0; i < MESON_CVBS_MODES_COUNT; ++i) {
struct meson_cvbs_mode *meson_mode = &meson_cvbs_modes[i];
- mode = drm_mode_duplicate(priv->drm, &meson_mode->mode);
+ mode = drm_mode_duplicate(priv->drm, meson_mode->mode);
if (!mode) {
dev_err(priv->dev, "Failed to create a new display mode\n");
return 0;
diff --git a/drivers/gpu/drm/meson/meson_encoder_cvbs.h b/drivers/gpu/drm/meson/meson_encoder_cvbs.h
index 61d9d183ce7f..26cefb202924 100644
--- a/drivers/gpu/drm/meson/meson_encoder_cvbs.h
+++ b/drivers/gpu/drm/meson/meson_encoder_cvbs.h
@@ -16,7 +16,7 @@
struct meson_cvbs_mode {
struct meson_cvbs_enci_mode *enci;
- struct drm_display_mode mode;
+ const struct drm_display_mode *mode;
};
#define MESON_CVBS_MODES_COUNT 2
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index a80ae9639e96..b4a440e2688c 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -394,6 +394,9 @@ struct drm_display_mode {
};
+extern const struct drm_display_mode drm_mode_480i;
+extern const struct drm_display_mode drm_mode_576i;
+
/**
* DRM_MODE_FMT - printf string for &struct drm_display_mode
*/


--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature