Re: [PATCH] drm: add backwards compatibility support for drm_kms_helper.edid_firmware

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Sep 18, 2017 at 09:20:03PM +0300, Jani Nikula wrote:
> Add drm_kms_helper.edid_firmware module parameter with param ops hooks
> to set drm.edid_firmware instead, for backwards compatibility.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 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>
> ---
>  drivers/gpu/drm/drm_edid_load.c         | 16 ++++++++++++++++
>  drivers/gpu/drm/drm_kms_helper_common.c | 28 ++++++++++++++++++++++++++++
>  include/drm/drm_edid.h                  |  2 ++
>  3 files changed, 46 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> index 1c0495acf341..a4915099aaa9 100644
> --- a/drivers/gpu/drm/drm_edid_load.c
> +++ b/drivers/gpu/drm/drm_edid_load.c
> @@ -31,6 +31,22 @@ 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 */
> +int __drm_set_edid_firmware_path(const char *path)
> +{
> +	scnprintf(edid_firmware, sizeof(edid_firmware), "%s", path);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(__drm_set_edid_firmware_path);
> +
> +/* Use only for backward compatibility with drm_kms_helper.edid_firmware */
> +int __drm_get_edid_firmware_path(char *buf, size_t bufsize)
> +{
> +	return scnprintf(buf, bufsize, "%s", edid_firmware);

I guess these could just use strlcpy() or something.

> +}
> +EXPORT_SYMBOL(__drm_get_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..5051c3aa4d5d 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,6 +34,33 @@ 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)
> +
> +/* Backward compatibility for drm_kms_helper.edid_firmware */
> +static int edid_firmware_set(const char *val, const struct kernel_param *kp)
> +{
> +	DRM_NOTE("drm_kms_firmware.edid_firmware is deprecated, please use drm.edid_firmware intead.\n");
> +
> +	return __drm_set_edid_firmware_path(val);
> +}
> +
> +static int edid_firmware_get(char *buffer, const struct kernel_param *kp)
> +{
> +	return __drm_get_edid_firmware_path(buffer, PAGE_SIZE);
> +}
> +
> +const struct kernel_param_ops edid_firmware_ops = {
> +	.set = edid_firmware_set,
> +	.get = edid_firmware_get,
> +};
> +
> +module_param_cb(edid_firmware, &edid_firmware_ops, NULL, 0644);

Do we want the __parm_check thing here? Looks like it's just some kind of
compile time type check though so not critical by any means.

Otherwise looks technically correct so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +__MODULE_PARM_TYPE(edid_firmware, "charp");
> +MODULE_PARM_DESC(edid_firmware,
> +		 "DEPRECATED. Use drm.edid_firmware module parameter instead.");
> +
> +#endif
> +
>  static int __init drm_kms_helper_init(void)
>  {
>  	int ret;
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 1e1908a6b1d6..6f35909b8add 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -341,6 +341,8 @@ 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);
> +int __drm_set_edid_firmware_path(const char *path);
> +int __drm_get_edid_firmware_path(char *buf, size_t bufsize);
>  #else
>  static inline struct edid *
>  drm_load_edid_firmware(struct drm_connector *connector)
> -- 
> 2.11.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel