The helper is generic and doesn't use the opaque EDID type struct drm_edid
and is also used by drivers that only support non-probeable displays, such
as fixed panels.
These drivers add a list of modes using drm_mode_probed_add() and then set
a preferred mode using the drm_set_preferred_mode() helper.
It seems more logical to have the helper definition in drm_modes.o instead
of drm_edid.o, since the former contains modes helper while the latter has
helpers to manage the EDID information.
Since both drm_edid.o and drm_modes.o object files are built-in the drm.o
object, there are no functional changes. But besides being a more logical
place for this helper, it could also allow to eventually make drm_edid.o
optional and not included in drm.o if only fixed panels must be supported
in a given system.
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
drivers/gpu/drm/drm_edid.c | 23 +----------------------
drivers/gpu/drm/drm_modes.c | 22 ++++++++++++++++++++++
include/drm/drm_edid.h | 2 --
include/drm/drm_modes.h | 2 ++
4 files changed, 25 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index cb4031d5dcbb..48dd2a0a0395 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -43,6 +43,7 @@
#include <drm/drm_edid.h>
#include <drm/drm_eld.h>
#include <drm/drm_encoder.h>
+#include <drm/drm_modes.h>
#include <drm/drm_print.h>
#include "drm_crtc_internal.h"
@@ -6989,28 +6990,6 @@ int drm_add_modes_noedid(struct drm_connector *connector,
}
EXPORT_SYMBOL(drm_add_modes_noedid);
-/**
- * drm_set_preferred_mode - Sets the preferred mode of a connector
- * @connector: connector whose mode list should be processed
- * @hpref: horizontal resolution of preferred mode
- * @vpref: vertical resolution of preferred mode
- *
- * Marks a mode as preferred if it matches the resolution specified by @hpref
- * and @vpref.
- */
-void drm_set_preferred_mode(struct drm_connector *connector,
- int hpref, int vpref)
-{
- struct drm_display_mode *mode;
-
- list_for_each_entry(mode, &connector->probed_modes, head) {
- if (mode->hdisplay == hpref &&
- mode->vdisplay == vpref)
- mode->type |= DRM_MODE_TYPE_PREFERRED;
- }
-}
-EXPORT_SYMBOL(drm_set_preferred_mode);
-
static bool is_hdmi2_sink(const struct drm_connector *connector)
{
/*
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index ac9a406250c5..01aa44e87534 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -2754,3 +2754,25 @@ bool drm_mode_is_420(const struct drm_display_info *display,
drm_mode_is_420_also(display, mode);
}
EXPORT_SYMBOL(drm_mode_is_420);
+
+/**
+ * drm_set_preferred_mode - Sets the preferred mode of a connector
+ * @connector: connector whose mode list should be processed
+ * @hpref: horizontal resolution of preferred mode
+ * @vpref: vertical resolution of preferred mode
+ *
+ * Marks a mode as preferred if it matches the resolution specified by @hpref
+ * and @vpref.
+ */
+void drm_set_preferred_mode(struct drm_connector *connector,
+ int hpref, int vpref)
+{
+ struct drm_display_mode *mode;
+
+ list_for_each_entry(mode, &connector->probed_modes, head) {
+ if (mode->hdisplay == hpref &&
+ mode->vdisplay == vpref)
+ mode->type |= DRM_MODE_TYPE_PREFERRED;
+ }
+}
+EXPORT_SYMBOL(drm_set_preferred_mode);
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 54cc6f04a708..5bd6b6eb6c57 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -426,8 +426,6 @@ enum hdmi_quantization_range
drm_default_rgb_quant_range(const struct drm_display_mode *mode);
int drm_add_modes_noedid(struct drm_connector *connector,
int hdisplay, int vdisplay);
-void drm_set_preferred_mode(struct drm_connector *connector,
- int hpref, int vpref);
int drm_edid_header_is_valid(const void *edid);
bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index c613f0abe9dc..b9bb92e4b029 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -467,6 +467,8 @@ bool drm_mode_is_420_also(const struct drm_display_info *display,
const struct drm_display_mode *mode);
bool drm_mode_is_420(const struct drm_display_info *display,
const struct drm_display_mode *mode);
+void drm_set_preferred_mode(struct drm_connector *connector,
+ int hpref, int vpref);
struct drm_display_mode *drm_analog_tv_mode(struct drm_device *dev,
enum drm_connector_tv_mode mode,
--
2.43.0
On Tue, 02 Jan 2024, Javier Martinez Canillas <[email protected]> wrote:
> The helper is generic and doesn't use the opaque EDID type struct drm_edid
> and is also used by drivers that only support non-probeable displays, such
> as fixed panels.
>
> These drivers add a list of modes using drm_mode_probed_add() and then set
> a preferred mode using the drm_set_preferred_mode() helper.
>
> It seems more logical to have the helper definition in drm_modes.o instead
> of drm_edid.o, since the former contains modes helper while the latter has
> helpers to manage the EDID information.
>
> Since both drm_edid.o and drm_modes.o object files are built-in the drm.o
> object, there are no functional changes. But besides being a more logical
> place for this helper, it could also allow to eventually make drm_edid.o
> optional and not included in drm.o if only fixed panels must be supported
> in a given system.
>
> Signed-off-by: Javier Martinez Canillas <[email protected]>
> ---
>
> drivers/gpu/drm/drm_edid.c | 23 +----------------------
> drivers/gpu/drm/drm_modes.c | 22 ++++++++++++++++++++++
> include/drm/drm_edid.h | 2 --
> include/drm/drm_modes.h | 2 ++
> 4 files changed, 25 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index cb4031d5dcbb..48dd2a0a0395 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -43,6 +43,7 @@
> #include <drm/drm_edid.h>
> #include <drm/drm_eld.h>
> #include <drm/drm_encoder.h>
> +#include <drm/drm_modes.h>
Unnecessary.
Other than that,
Reviewed-by: Jani Nikula <[email protected]>
> #include <drm/drm_print.h>
>
> #include "drm_crtc_internal.h"
> @@ -6989,28 +6990,6 @@ int drm_add_modes_noedid(struct drm_connector *connector,
> }
> EXPORT_SYMBOL(drm_add_modes_noedid);
>
> -/**
> - * drm_set_preferred_mode - Sets the preferred mode of a connector
> - * @connector: connector whose mode list should be processed
> - * @hpref: horizontal resolution of preferred mode
> - * @vpref: vertical resolution of preferred mode
> - *
> - * Marks a mode as preferred if it matches the resolution specified by @hpref
> - * and @vpref.
> - */
> -void drm_set_preferred_mode(struct drm_connector *connector,
> - int hpref, int vpref)
> -{
> - struct drm_display_mode *mode;
> -
> - list_for_each_entry(mode, &connector->probed_modes, head) {
> - if (mode->hdisplay == hpref &&
> - mode->vdisplay == vpref)
> - mode->type |= DRM_MODE_TYPE_PREFERRED;
> - }
> -}
> -EXPORT_SYMBOL(drm_set_preferred_mode);
> -
> static bool is_hdmi2_sink(const struct drm_connector *connector)
> {
> /*
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index ac9a406250c5..01aa44e87534 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2754,3 +2754,25 @@ bool drm_mode_is_420(const struct drm_display_info *display,
> drm_mode_is_420_also(display, mode);
> }
> EXPORT_SYMBOL(drm_mode_is_420);
> +
> +/**
> + * drm_set_preferred_mode - Sets the preferred mode of a connector
> + * @connector: connector whose mode list should be processed
> + * @hpref: horizontal resolution of preferred mode
> + * @vpref: vertical resolution of preferred mode
> + *
> + * Marks a mode as preferred if it matches the resolution specified by @hpref
> + * and @vpref.
> + */
> +void drm_set_preferred_mode(struct drm_connector *connector,
> + int hpref, int vpref)
> +{
> + struct drm_display_mode *mode;
> +
> + list_for_each_entry(mode, &connector->probed_modes, head) {
> + if (mode->hdisplay == hpref &&
> + mode->vdisplay == vpref)
> + mode->type |= DRM_MODE_TYPE_PREFERRED;
> + }
> +}
> +EXPORT_SYMBOL(drm_set_preferred_mode);
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 54cc6f04a708..5bd6b6eb6c57 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -426,8 +426,6 @@ enum hdmi_quantization_range
> drm_default_rgb_quant_range(const struct drm_display_mode *mode);
> int drm_add_modes_noedid(struct drm_connector *connector,
> int hdisplay, int vdisplay);
> -void drm_set_preferred_mode(struct drm_connector *connector,
> - int hpref, int vpref);
>
> int drm_edid_header_is_valid(const void *edid);
> bool drm_edid_block_valid(u8 *raw_edid, int block, bool print_bad_edid,
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index c613f0abe9dc..b9bb92e4b029 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -467,6 +467,8 @@ bool drm_mode_is_420_also(const struct drm_display_info *display,
> const struct drm_display_mode *mode);
> bool drm_mode_is_420(const struct drm_display_info *display,
> const struct drm_display_mode *mode);
> +void drm_set_preferred_mode(struct drm_connector *connector,
> + int hpref, int vpref);
>
> struct drm_display_mode *drm_analog_tv_mode(struct drm_device *dev,
> enum drm_connector_tv_mode mode,
--
Jani Nikula, Intel
Jani Nikula <[email protected]> writes:
Hello Jani,
> On Tue, 02 Jan 2024, Javier Martinez Canillas <[email protected]> wrote:
>> The helper is generic and doesn't use the opaque EDID type struct drm_edid
>> and is also used by drivers that only support non-probeable displays, such
>> as fixed panels.
>>
>> These drivers add a list of modes using drm_mode_probed_add() and then set
>> a preferred mode using the drm_set_preferred_mode() helper.
>>
>> It seems more logical to have the helper definition in drm_modes.o instead
>> of drm_edid.o, since the former contains modes helper while the latter has
>> helpers to manage the EDID information.
>>
>> Since both drm_edid.o and drm_modes.o object files are built-in the drm.o
>> object, there are no functional changes. But besides being a more logical
>> place for this helper, it could also allow to eventually make drm_edid.o
>> optional and not included in drm.o if only fixed panels must be supported
>> in a given system.
>>
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
>> ---
>>
>> drivers/gpu/drm/drm_edid.c | 23 +----------------------
>> drivers/gpu/drm/drm_modes.c | 22 ++++++++++++++++++++++
>> include/drm/drm_edid.h | 2 --
>> include/drm/drm_modes.h | 2 ++
>> 4 files changed, 25 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index cb4031d5dcbb..48dd2a0a0395 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -43,6 +43,7 @@
>> #include <drm/drm_edid.h>
>> #include <drm/drm_eld.h>
>> #include <drm/drm_encoder.h>
>> +#include <drm/drm_modes.h>
>
> Unnecessary.
>
Indeed. I could swear that saw drm_set_preferred_mode() being called
somewhere in drm_edid.c but looking again I see that's not the case.
> Other than that,
>
> Reviewed-by: Jani Nikula <[email protected]>
>
Thanks. I'll post a v2 that drops the unnecessary header include.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat