Re: [PATCH v2 4/4] drm: handle override edid and firmware EDID at drm_do_get_edid() level

[Ville Syrjälä]

On Mon, Feb 27, 2017 at 05:19:21PM +0100, Daniel Vetter wrote:
> On Mon, Feb 27, 2017 at 05:09:44PM +0200, Ville Syrjälä wrote:
> > On Sun, Feb 26, 2017 at 10:22:42PM +0100, Daniel Vetter wrote:
> > > On Fri, Feb 17, 2017 at 05:20:54PM +0200, Jani Nikula wrote:
> > > > Handle debugfs override edid and firmware edid at the low level to
> > > > transparently and completely replace the real edid. Previously, we
> > > > practically only used the modes from the override EDID, and none of the
> > > > other data. This also prevents actual EDID reads when the EDID is to be
> > > > overridden, but retains the DDC probe.
> > > > 
> > > > Move firmware EDID loading from helper to core, as the functionality
> > > > moves to lower level as well. This will result in a change of module
> > > > parameter from drm_kms_helper.edid_firmware to drm.edid_firmware, which
> > > > arguably makes more sense anyway.
> > > > 
> > > > FIXME: validate override edid, deduplicate firmware edid validation.
> > > > 
> > > > v2: move firmware loading to core
> > > > 
> > > > Signed-off-by: Jani Nikula 
> > > > ---
> > > >  drivers/gpu/drm/Kconfig|  2 +-
> > > >  drivers/gpu/drm/Makefile   |  2 +-
> > > >  drivers/gpu/drm/drm_edid.c | 15 +++
> > > >  drivers/gpu/drm/drm_probe_helper.c | 19 +--
> > > >  4 files changed, 18 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > > index 90bc65d07a35..f983ef60299c 100644
> > > > --- a/drivers/gpu/drm/Kconfig
> > > > +++ b/drivers/gpu/drm/Kconfig
> > > > @@ -101,7 +101,7 @@ config DRM_FBDEV_EMULATION
> > > >  
> > > >  config DRM_LOAD_EDID_FIRMWARE
> > > > bool "Allow to specify an EDID data set instead of probing for 
> > > > it"
> > > > -   depends on DRM_KMS_HELPER
> > > > +   depends on DRM
> > > > help
> > > >   Say Y here, if you want to use EDID data to be loaded from the
> > > >   /lib/firmware directory or one of the provided built-in
> > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > > index 92de3991fa56..a10ac095608f 100644
> > > > --- a/drivers/gpu/drm/Makefile
> > > > +++ b/drivers/gpu/drm/Makefile
> > > > @@ -27,13 +27,13 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o
> > > >  drm-$(CONFIG_OF) += drm_of.o
> > > >  drm-$(CONFIG_AGP) += drm_agpsupport.o
> > > >  drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
> > > > +drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> > > >  
> > > >  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o 
> > > > drm_probe_helper.o \
> > > > drm_plane_helper.o drm_dp_mst_topology.o 
> > > > drm_atomic_helper.o \
> > > > drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
> > > > drm_simple_kms_helper.o drm_modeset_helper.o
> > > >  
> > > > -drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> > > >  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> > > >  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> > > >  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > index e1743ab276dc..4007998d5ce3 100644
> > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > @@ -1309,6 +1309,10 @@ static void connector_bad_edid(struct 
> > > > drm_connector *connector,
> > > >   * level, drivers must make all reasonable efforts to expose it as an 
> > > > I2C
> > > >   * adapter and use drm_get_edid() instead of abusing this function.
> > > >   *
> > > > + * The EDID may be overridden using debugfs override_edid or firmare 
> > > > EDID
> > > > + * (drm_load_edid_firmware()), in this priority order. Having either 
> > > > of them
> > > > + * bypasses actual EDID reads.
> > > > + *
> > > >   * Return: Pointer to valid EDID or NULL if we couldn't find any.
> > > >   */
> > > >  struct edid *drm_do_get_edid(struct drm_connector *connector,
> > > > @@ -1318,6 +1322,17 @@ struct edid *drm_do_get_edid(struct 
> > > > drm_connector *connector,
> > > >  {
> > > > int i, j = 0, valid_extensions = 0;
> > > > u8 *edid, *new;
> > > > +   struct edid *override = NULL;
> > > > +
> > > > +   if (connector->override_edid)
> > > > +   override = drm_edid_duplicate((const struct edid *)
> > > > + 
> > > > connector->edid_blob_ptr->data);
> > > > +
> > > > +   if (!override)
> > > > +   override = drm_load_edid_firmware(connector);
> > > > +
> > > > +   if (!IS_ERR_OR_NULL(override))
> > > > +   return override;
> > > >  
> > > > if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
> > > > return NULL;
> > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c 
> > > > b/drivers/gpu/drm/drm_probe_helper.c
> > > 

Re: [PATCH v2 4/4] drm: handle override edid and firmware EDID at drm_do_get_edid() level

[Daniel Vetter]

On Mon, Feb 27, 2017 at 05:09:44PM +0200, Ville Syrjälä wrote:
> On Sun, Feb 26, 2017 at 10:22:42PM +0100, Daniel Vetter wrote:
> > On Fri, Feb 17, 2017 at 05:20:54PM +0200, Jani Nikula wrote:
> > > Handle debugfs override edid and firmware edid at the low level to
> > > transparently and completely replace the real edid. Previously, we
> > > practically only used the modes from the override EDID, and none of the
> > > other data. This also prevents actual EDID reads when the EDID is to be
> > > overridden, but retains the DDC probe.
> > > 
> > > Move firmware EDID loading from helper to core, as the functionality
> > > moves to lower level as well. This will result in a change of module
> > > parameter from drm_kms_helper.edid_firmware to drm.edid_firmware, which
> > > arguably makes more sense anyway.
> > > 
> > > FIXME: validate override edid, deduplicate firmware edid validation.
> > > 
> > > v2: move firmware loading to core
> > > 
> > > Signed-off-by: Jani Nikula 
> > > ---
> > >  drivers/gpu/drm/Kconfig|  2 +-
> > >  drivers/gpu/drm/Makefile   |  2 +-
> > >  drivers/gpu/drm/drm_edid.c | 15 +++
> > >  drivers/gpu/drm/drm_probe_helper.c | 19 +--
> > >  4 files changed, 18 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > index 90bc65d07a35..f983ef60299c 100644
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -101,7 +101,7 @@ config DRM_FBDEV_EMULATION
> > >  
> > >  config DRM_LOAD_EDID_FIRMWARE
> > >   bool "Allow to specify an EDID data set instead of probing for it"
> > > - depends on DRM_KMS_HELPER
> > > + depends on DRM
> > >   help
> > > Say Y here, if you want to use EDID data to be loaded from the
> > > /lib/firmware directory or one of the provided built-in
> > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > index 92de3991fa56..a10ac095608f 100644
> > > --- a/drivers/gpu/drm/Makefile
> > > +++ b/drivers/gpu/drm/Makefile
> > > @@ -27,13 +27,13 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o
> > >  drm-$(CONFIG_OF) += drm_of.o
> > >  drm-$(CONFIG_AGP) += drm_agpsupport.o
> > >  drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
> > > +drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> > >  
> > >  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o 
> > > \
> > >   drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> > >   drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
> > >   drm_simple_kms_helper.o drm_modeset_helper.o
> > >  
> > > -drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> > >  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> > >  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> > >  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index e1743ab276dc..4007998d5ce3 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -1309,6 +1309,10 @@ static void connector_bad_edid(struct 
> > > drm_connector *connector,
> > >   * level, drivers must make all reasonable efforts to expose it as an I2C
> > >   * adapter and use drm_get_edid() instead of abusing this function.
> > >   *
> > > + * The EDID may be overridden using debugfs override_edid or firmare EDID
> > > + * (drm_load_edid_firmware()), in this priority order. Having either of 
> > > them
> > > + * bypasses actual EDID reads.
> > > + *
> > >   * Return: Pointer to valid EDID or NULL if we couldn't find any.
> > >   */
> > >  struct edid *drm_do_get_edid(struct drm_connector *connector,
> > > @@ -1318,6 +1322,17 @@ struct edid *drm_do_get_edid(struct drm_connector 
> > > *connector,
> > >  {
> > >   int i, j = 0, valid_extensions = 0;
> > >   u8 *edid, *new;
> > > + struct edid *override = NULL;
> > > +
> > > + if (connector->override_edid)
> > > + override = drm_edid_duplicate((const struct edid *)
> > > +   connector->edid_blob_ptr->data);
> > > +
> > > + if (!override)
> > > + override = drm_load_edid_firmware(connector);
> > > +
> > > + if (!IS_ERR_OR_NULL(override))
> > > + return override;
> > >  
> > >   if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
> > >   return NULL;
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c 
> > > b/drivers/gpu/drm/drm_probe_helper.c
> > > index 358957118ca9..871326cbc465 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -199,8 +199,6 @@ drm_connector_detect(struct drm_connector *connector, 
> > > bool force)
> > >   *drm_mode_probed_add(). New modes start their life with status as 
> > > OK.
> > >   *Modes are added from a single source using the following priority 
> > > order.
> > >   *
> 

Re: [PATCH v2 4/4] drm: handle override edid and firmware EDID at drm_do_get_edid() level

[Ville Syrjälä]

On Sun, Feb 26, 2017 at 10:22:42PM +0100, Daniel Vetter wrote:
> On Fri, Feb 17, 2017 at 05:20:54PM +0200, Jani Nikula wrote:
> > Handle debugfs override edid and firmware edid at the low level to
> > transparently and completely replace the real edid. Previously, we
> > practically only used the modes from the override EDID, and none of the
> > other data. This also prevents actual EDID reads when the EDID is to be
> > overridden, but retains the DDC probe.
> > 
> > Move firmware EDID loading from helper to core, as the functionality
> > moves to lower level as well. This will result in a change of module
> > parameter from drm_kms_helper.edid_firmware to drm.edid_firmware, which
> > arguably makes more sense anyway.
> > 
> > FIXME: validate override edid, deduplicate firmware edid validation.
> > 
> > v2: move firmware loading to core
> > 
> > Signed-off-by: Jani Nikula 
> > ---
> >  drivers/gpu/drm/Kconfig|  2 +-
> >  drivers/gpu/drm/Makefile   |  2 +-
> >  drivers/gpu/drm/drm_edid.c | 15 +++
> >  drivers/gpu/drm/drm_probe_helper.c | 19 +--
> >  4 files changed, 18 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 90bc65d07a35..f983ef60299c 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -101,7 +101,7 @@ config DRM_FBDEV_EMULATION
> >  
> >  config DRM_LOAD_EDID_FIRMWARE
> > bool "Allow to specify an EDID data set instead of probing for it"
> > -   depends on DRM_KMS_HELPER
> > +   depends on DRM
> > help
> >   Say Y here, if you want to use EDID data to be loaded from the
> >   /lib/firmware directory or one of the provided built-in
> > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > index 92de3991fa56..a10ac095608f 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -27,13 +27,13 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o
> >  drm-$(CONFIG_OF) += drm_of.o
> >  drm-$(CONFIG_AGP) += drm_agpsupport.o
> >  drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
> > +drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> >  
> >  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
> > drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
> > drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
> > drm_simple_kms_helper.o drm_modeset_helper.o
> >  
> > -drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> >  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> >  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> >  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index e1743ab276dc..4007998d5ce3 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -1309,6 +1309,10 @@ static void connector_bad_edid(struct drm_connector 
> > *connector,
> >   * level, drivers must make all reasonable efforts to expose it as an I2C
> >   * adapter and use drm_get_edid() instead of abusing this function.
> >   *
> > + * The EDID may be overridden using debugfs override_edid or firmare EDID
> > + * (drm_load_edid_firmware()), in this priority order. Having either of 
> > them
> > + * bypasses actual EDID reads.
> > + *
> >   * Return: Pointer to valid EDID or NULL if we couldn't find any.
> >   */
> >  struct edid *drm_do_get_edid(struct drm_connector *connector,
> > @@ -1318,6 +1322,17 @@ struct edid *drm_do_get_edid(struct drm_connector 
> > *connector,
> >  {
> > int i, j = 0, valid_extensions = 0;
> > u8 *edid, *new;
> > +   struct edid *override = NULL;
> > +
> > +   if (connector->override_edid)
> > +   override = drm_edid_duplicate((const struct edid *)
> > + connector->edid_blob_ptr->data);
> > +
> > +   if (!override)
> > +   override = drm_load_edid_firmware(connector);
> > +
> > +   if (!IS_ERR_OR_NULL(override))
> > +   return override;
> >  
> > if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
> > return NULL;
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c 
> > b/drivers/gpu/drm/drm_probe_helper.c
> > index 358957118ca9..871326cbc465 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -199,8 +199,6 @@ drm_connector_detect(struct drm_connector *connector, 
> > bool force)
> >   *drm_mode_probed_add(). New modes start their life with status as OK.
> >   *Modes are added from a single source using the following priority 
> > order.
> >   *
> > - *- debugfs 'override_edid' (used for testing only)
> > - *- firmware EDID (drm_load_edid_firmware())
> >   *- _connector_helper_funcs.get_modes vfunc
> >   *- if the connector status is connector_status_connected, standard

Re: [PATCH v2 4/4] drm: handle override edid and firmware EDID at drm_do_get_edid() level

[Daniel Vetter]

On Fri, Feb 17, 2017 at 05:20:54PM +0200, Jani Nikula wrote:
> Handle debugfs override edid and firmware edid at the low level to
> transparently and completely replace the real edid. Previously, we
> practically only used the modes from the override EDID, and none of the
> other data. This also prevents actual EDID reads when the EDID is to be
> overridden, but retains the DDC probe.
> 
> Move firmware EDID loading from helper to core, as the functionality
> moves to lower level as well. This will result in a change of module
> parameter from drm_kms_helper.edid_firmware to drm.edid_firmware, which
> arguably makes more sense anyway.
> 
> FIXME: validate override edid, deduplicate firmware edid validation.
> 
> v2: move firmware loading to core
> 
> Signed-off-by: Jani Nikula 
> ---
>  drivers/gpu/drm/Kconfig|  2 +-
>  drivers/gpu/drm/Makefile   |  2 +-
>  drivers/gpu/drm/drm_edid.c | 15 +++
>  drivers/gpu/drm/drm_probe_helper.c | 19 +--
>  4 files changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 90bc65d07a35..f983ef60299c 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -101,7 +101,7 @@ config DRM_FBDEV_EMULATION
>  
>  config DRM_LOAD_EDID_FIRMWARE
>   bool "Allow to specify an EDID data set instead of probing for it"
> - depends on DRM_KMS_HELPER
> + depends on DRM
>   help
> Say Y here, if you want to use EDID data to be loaded from the
> /lib/firmware directory or one of the provided built-in
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 92de3991fa56..a10ac095608f 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -27,13 +27,13 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o
>  drm-$(CONFIG_OF) += drm_of.o
>  drm-$(CONFIG_AGP) += drm_agpsupport.o
>  drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
> +drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  
>  drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
>   drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
>   drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
>   drm_simple_kms_helper.o drm_modeset_helper.o
>  
> -drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>  drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
>  drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
>  drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index e1743ab276dc..4007998d5ce3 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1309,6 +1309,10 @@ static void connector_bad_edid(struct drm_connector 
> *connector,
>   * level, drivers must make all reasonable efforts to expose it as an I2C
>   * adapter and use drm_get_edid() instead of abusing this function.
>   *
> + * The EDID may be overridden using debugfs override_edid or firmare EDID
> + * (drm_load_edid_firmware()), in this priority order. Having either of them
> + * bypasses actual EDID reads.
> + *
>   * Return: Pointer to valid EDID or NULL if we couldn't find any.
>   */
>  struct edid *drm_do_get_edid(struct drm_connector *connector,
> @@ -1318,6 +1322,17 @@ struct edid *drm_do_get_edid(struct drm_connector 
> *connector,
>  {
>   int i, j = 0, valid_extensions = 0;
>   u8 *edid, *new;
> + struct edid *override = NULL;
> +
> + if (connector->override_edid)
> + override = drm_edid_duplicate((const struct edid *)
> +   connector->edid_blob_ptr->data);
> +
> + if (!override)
> + override = drm_load_edid_firmware(connector);
> +
> + if (!IS_ERR_OR_NULL(override))
> + return override;
>  
>   if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
>   return NULL;
> diff --git a/drivers/gpu/drm/drm_probe_helper.c 
> b/drivers/gpu/drm/drm_probe_helper.c
> index 358957118ca9..871326cbc465 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -199,8 +199,6 @@ drm_connector_detect(struct drm_connector *connector, 
> bool force)
>   *drm_mode_probed_add(). New modes start their life with status as OK.
>   *Modes are added from a single source using the following priority 
> order.
>   *
> - *- debugfs 'override_edid' (used for testing only)
> - *- firmware EDID (drm_load_edid_firmware())
>   *- _connector_helper_funcs.get_modes vfunc
>   *- if the connector status is connector_status_connected, standard
>   *  VESA DMT modes up to 1024x768 are automatically added
> @@ -305,22 +303,7 @@ int drm_helper_probe_single_connector_modes(struct 
> drm_connector *connector,
>   goto prune;
>   }
>  
> - if