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

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 18 Sep 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 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.

strlcpy() returns strlen(source) while scnprintf() returns the number of
chars written to destination (minus the terminating NUL), and that's
what the kernel param ops expect. I took this directly from
kernel/params.c, and preferred to use the same for both get and set.

>
>> +}
>> +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.

It's useless here, because we don't actually store the parameter here,
and just pass NULL.

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

Thanks,
Jani.

>
>> +__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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel