Re: [RFC PATCH 6/7] drm: add new drm_mode_connector_get_edid to do drm_get_edid and friends

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Dec 27, 2016 at 06:21:53PM +0200, Jani Nikula wrote:
> Add a replacement for drm_get_edid that handles override and firmware
> EDID at the low level, and handles EDID property update, ELD update, and
> adds modes. This allows for a more transparent EDID override, and makes
> it possible to unify the drivers better. It also allows reusing the EDID
> cached in the property, and only probing the DDC.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_connector.c    | 60 ++++++++++++++++++++++++++++++++++++++

It's a helper, but placed in drm.ko core. Moving it into drm_kms_helper.ko
should remove the need for some of the experts, and that way we should
also be able to unload modules again.

>  drivers/gpu/drm/drm_probe_helper.c |  7 +++++
>  include/drm/drm_connector.h        |  6 ++++
>  3 files changed, 73 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 3115db2ae6b1..b9636c98dbf3 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1086,6 +1086,66 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_mode_connector_update_edid_property);
>  
> +/**
> + * drm_mode_connector_get_edid - do it all replacement for drm_get_edid()
> + * @connector: connector to probe
> + * @adapter: I2C adapter to use for DDC
> + * @edid_out: if non-NULL, get EDID here, must be kfree'd by caller
> + * @no_cache: false to allow using cached EDID, true to force read
> + *
> + * drm_get_edid() on steroids. Handle debugfs override edid, firmware edid, and
> + * caching at the low level. Add modes, update ELD and EDID property. Using this
> + * prevents override/firmware edid usage at the higher level.
> + *
> + * @return: Number of modes from drm_add_edid_modes().
> + */
> +int drm_mode_connector_get_edid(struct drm_connector *connector,
> +				struct i2c_adapter *adapter,
> +				struct edid **edid_out,
> +				bool no_cache)
> +{
> +	struct drm_property_blob *prop = connector->edid_blob_ptr;
> +	struct edid *edid = NULL;
> +	int count = 0;
> +
> +	/*
> +	 * If a driver uses this function on a connector, override/firmware edid
> +	 * will only be used at this level.
> +	 */
> +	connector->low_level_override = true;
> +
> +	if ((connector->override_edid || !no_cache) && prop && prop->data)
> +		edid = drm_edid_duplicate((struct edid *) prop->data);
> +
> +	if (!edid) {
> +		edid = drm_load_edid_firmware(connector);
> +		if (IS_ERR(edid))
> +			edid = NULL;
> +	}
> +
> +	if (edid) {
> +		/* Require successful probe for override/cached/firmware EDID */
> +		if (drm_probe_ddc(adapter))

This doesn't take into account hpd or connector status overwriting. Not
sure how to solve that, but probably we need to push those down a few
layers, too.

There's also the annoying split of ->detect vs. ->get_modes, and I think
if you want to clean up this entire mess, then unifying it all would be
really good. With caching and everything there's no real need to split
things into parts.

I'm not sure how this all should look like.
-Daniel

> +			drm_get_displayid(connector, edid);
> +		else
> +			edid = NULL;
> +	} else {
> +		edid = drm_get_edid(connector, adapter);
> +	}
> +
> +	count = drm_add_edid_modes(connector, edid);
> +	drm_edid_to_eld(connector, edid);
> +	drm_mode_connector_update_edid_property(connector, edid);
> +
> +	if (edid_out)
> +		*edid_out = edid;
> +	else
> +		kfree(edid);
> +
> +	return count;
> +}
> +EXPORT_SYMBOL(drm_mode_connector_get_edid);
> +
>  int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
>  				    struct drm_property *property,
>  				    uint64_t value)
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 431349673846..20a175a775d6 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -167,6 +167,13 @@ static bool bypass_edid(struct drm_connector *connector, int *count)
>  {
>  	struct edid *edid;
>  
> +	/*
> +	 * If a driver uses drm_connector_get_edid() on a connector,
> +	 * override/firmware edid will only be used at that level.
> +	 */
> +	if (connector->low_level_override)
> +		return false;
> +
>  	if (connector->override_edid) {
>  		edid = (struct edid *) connector->edid_blob_ptr->data;
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 6e352a0b5c81..0c85ddc422de 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -25,6 +25,7 @@
>  
>  #include <linux/list.h>
>  #include <linux/ctype.h>
> +#include <linux/i2c.h>
>  #include <drm/drm_mode_object.h>
>  
>  #include <uapi/drm/drm_mode.h>
> @@ -727,6 +728,7 @@ struct drm_connector {
>  	/* forced on connector */
>  	struct drm_cmdline_mode cmdline_mode;
>  	enum drm_connector_force force;
> +	bool low_level_override;
>  	bool override_edid;
>  
>  #define DRM_CONNECTOR_MAX_ENCODER 3
> @@ -837,6 +839,10 @@ int drm_mode_connector_set_path_property(struct drm_connector *connector,
>  int drm_mode_connector_set_tile_property(struct drm_connector *connector);
>  int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>  					    const struct edid *edid);
> +int drm_mode_connector_get_edid(struct drm_connector *connector,
> +				struct i2c_adapter *adapter,
> +				struct edid **edid_out,
> +				bool no_cache);
>  
>  /**
>   * struct drm_tile_group - Tile group metadata
> -- 
> 2.1.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx