[v3,2/2] drm: add backwards compatibility support for drm_kms_helper.edid_firmware

Submitted by Jani Nikula on Sept. 12, 2017, 8:19 a.m.

Details

Message ID 4968bb2bdd416e8dd84b3059b7c9843732dcef0e.1505203831.git.jani.nikula@intel.com
State New
Headers show
Series "drm/edid: transparent low-level override/firmware EDIDs" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Jani Nikula Sept. 12, 2017, 8:19 a.m.
If drm_kms_helper.edid_firmware module parameter is set at
drm_kms_helper probe time, update the new drm.edid_firmware parameter
for backwards compatibility.

The drm_kms_helper.edid_firmware is now read-only in sysfs to prevent
runtime updates.

This is a sort of easy middle ground; adding a full runtime support via
/sys/module/drm_kms_helper/parameters/edid_firmware would be possible,
but considerably more and uglier code.

Cc: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

I'm wondering if the change from drm_kms_helper.edid_firmware to
drm.edid_firmware is enough of an ABI breakage to warrant something like
this patch.
---
 drivers/gpu/drm/drm_edid_load.c         |  7 +++++++
 drivers/gpu/drm/drm_kms_helper_common.c | 19 +++++++++++++++++++
 include/drm/drm_edid.h                  |  2 ++
 3 files changed, 28 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 1c0495acf341..a94005529cd4 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -31,6 +31,13 @@  module_param_string(edid_firmware, edid_firmware, sizeof(edid_firmware), 0644);
 MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob "
 	"from built-in data or /lib/firmware instead. ");
 
+/* Use only for backward compatibility with drm_kms_helper.edid_firmware */
+void __drm_set_edid_firmware_path(const char *path)
+{
+	strlcpy(edid_firmware, path, sizeof(edid_firmware));
+}
+EXPORT_SYMBOL(__drm_set_edid_firmware_path);
+
 #define GENERIC_EDIDS 6
 static const char * const generic_edid_name[GENERIC_EDIDS] = {
 	"edid/800x600.bin",
diff --git a/drivers/gpu/drm/drm_kms_helper_common.c b/drivers/gpu/drm/drm_kms_helper_common.c
index 6e35a56a6102..6130199a83b6 100644
--- a/drivers/gpu/drm/drm_kms_helper_common.c
+++ b/drivers/gpu/drm/drm_kms_helper_common.c
@@ -26,6 +26,7 @@ 
  */
 
 #include <linux/module.h>
+#include <drm/drmP.h>
 
 #include "drm_crtc_helper_internal.h"
 
@@ -33,10 +34,28 @@  MODULE_AUTHOR("David Airlie, Jesse Barnes");
 MODULE_DESCRIPTION("DRM KMS helper");
 MODULE_LICENSE("GPL and additional rights");
 
+#if IS_ENABLED(CONFIG_DRM_LOAD_EDID_FIRMWARE)
+static char edid_firmware[PATH_MAX];
+module_param_string(edid_firmware, edid_firmware, sizeof(edid_firmware), 0444);
+MODULE_PARM_DESC(edid_firmware, "DEPRECATED. Use drm.edid_firmware instead.");
+static inline void legacy_set_edid_firmare_path(void)
+{
+	if (edid_firmware[0]) {
+		DRM_NOTE("drm_kms_firmware.edid_firmware is deprecated, "
+			 "please use drm.edid_firmware intead.\n");
+		__drm_set_edid_firmware_path(edid_firmware);
+	}
+}
+#else
+static inline void legacy_set_edid_firmare_path(void) {}
+#endif
+
 static int __init drm_kms_helper_init(void)
 {
 	int ret;
 
+	legacy_set_edid_firmare_path();
+
 	/* Call init functions from specific kms helpers here */
 	ret = drm_fb_helper_modinit();
 	if (ret < 0)
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 1e1908a6b1d6..cdfb793255f0 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -341,12 +341,14 @@  int drm_av_sync_delay(struct drm_connector *connector,
 
 #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
 struct edid *drm_load_edid_firmware(struct drm_connector *connector);
+void __drm_set_edid_firmware_path(const char *path);
 #else
 static inline struct edid *
 drm_load_edid_firmware(struct drm_connector *connector)
 {
 	return ERR_PTR(-ENOENT);
 }
+static inline void __drm_set_edid_firmware_path(const char *path) {}
 #endif
 
 int

Comments

On Tue, Sep 12, 2017 at 11:19:27AM +0300, Jani Nikula wrote:
> If drm_kms_helper.edid_firmware module parameter is set at
> drm_kms_helper probe time, update the new drm.edid_firmware parameter
> for backwards compatibility.
> 
> The drm_kms_helper.edid_firmware is now read-only in sysfs to prevent
> runtime updates.
> 
> This is a sort of easy middle ground; adding a full runtime support via
> /sys/module/drm_kms_helper/parameters/edid_firmware would be possible,
> but considerably more and uglier code.

Hmm. Wouldn't you just have to have custom kernel_param_ops and
that's about it? Seems like that could be a bit cleaner.

> 
> Cc: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> 
> ---
> 
> I'm wondering if the change from drm_kms_helper.edid_firmware to
> drm.edid_firmware is enough of an ABI breakage to warrant something like
> this patch.
> ---
>  drivers/gpu/drm/drm_edid_load.c         |  7 +++++++
>  drivers/gpu/drm/drm_kms_helper_common.c | 19 +++++++++++++++++++
>  include/drm/drm_edid.h                  |  2 ++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 1c0495acf341..a94005529cd4 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -31,6 +31,13 @@ module_param_string(edid_firmware, edid_firmware, sizeof(edid_firmware), 0644);
>  MODULE_PARM_DESC(edid_firmware, "Do not probe monitor, use specified EDID blob "
>  	"from built-in data or /lib/firmware instead. ");
>  
> +/* Use only for backward compatibility with drm_kms_helper.edid_firmware */
> +void __drm_set_edid_firmware_path(const char *path)
> +{
> +	strlcpy(edid_firmware, path, sizeof(edid_firmware));
> +}
> +EXPORT_SYMBOL(__drm_set_edid_firmware_path);
> +
>  #define GENERIC_EDIDS 6
>  static const char * const generic_edid_name[GENERIC_EDIDS] = {
>  	"edid/800x600.bin",
> diff --git a/drivers/gpu/drm/drm_kms_helper_common.c b/drivers/gpu/drm/drm_kms_helper_common.c
> index 6e35a56a6102..6130199a83b6 100644
> --- a/drivers/gpu/drm/drm_kms_helper_common.c
> +++ b/drivers/gpu/drm/drm_kms_helper_common.c
> @@ -26,6 +26,7 @@
>   */
>  
>  #include <linux/module.h>
> +#include <drm/drmP.h>
>  
>  #include "drm_crtc_helper_internal.h"
>  
> @@ -33,10 +34,28 @@ MODULE_AUTHOR("David Airlie, Jesse Barnes");
>  MODULE_DESCRIPTION("DRM KMS helper");
>  MODULE_LICENSE("GPL and additional rights");
>  
> +#if IS_ENABLED(CONFIG_DRM_LOAD_EDID_FIRMWARE)
> +static char edid_firmware[PATH_MAX];
> +module_param_string(edid_firmware, edid_firmware, sizeof(edid_firmware), 0444);
> +MODULE_PARM_DESC(edid_firmware, "DEPRECATED. Use drm.edid_firmware instead.");
> +static inline void legacy_set_edid_firmare_path(void)
> +{
> +	if (edid_firmware[0]) {
> +		DRM_NOTE("drm_kms_firmware.edid_firmware is deprecated, "
> +			 "please use drm.edid_firmware intead.\n");
> +		__drm_set_edid_firmware_path(edid_firmware);
> +	}
> +}
> +#else
> +static inline void legacy_set_edid_firmare_path(void) {}
> +#endif
> +
>  static int __init drm_kms_helper_init(void)
>  {
>  	int ret;
>  
> +	legacy_set_edid_firmare_path();
> +
>  	/* Call init functions from specific kms helpers here */
>  	ret = drm_fb_helper_modinit();
>  	if (ret < 0)
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 1e1908a6b1d6..cdfb793255f0 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -341,12 +341,14 @@ int drm_av_sync_delay(struct drm_connector *connector,
>  
>  #ifdef CONFIG_DRM_LOAD_EDID_FIRMWARE
>  struct edid *drm_load_edid_firmware(struct drm_connector *connector);
> +void __drm_set_edid_firmware_path(const char *path);
>  #else
>  static inline struct edid *
>  drm_load_edid_firmware(struct drm_connector *connector)
>  {
>  	return ERR_PTR(-ENOENT);
>  }
> +static inline void __drm_set_edid_firmware_path(const char *path) {}
>  #endif
>  
>  int
> -- 
> 2.11.0
On Fri, 15 Sep 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> Hmm. Wouldn't you just have to have custom kernel_param_ops and
> that's about it? Seems like that could be a bit cleaner.

Here's a shot at that. Completely untested, but seems like this should
do the trick, and is less complex than I anticipated.

BR,
Jani.