| Message ID | 09a4c542e015f8f5de46e4ba6a393b20490b15e1.1482854862.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Tue, 27 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> I'm not sure how this all should look like.
This we definitely agree on, and hence the RFC. :)
I'm pretty sure it should *not* look like it currently does.
BR,
Jani.
On Wed, Dec 28, 2016 at 10:39:17AM +0200, Jani Nikula wrote: > On Tue, 27 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote: > > I'm not sure how this all should look like. > > This we definitely agree on, and hence the RFC. :) > > I'm pretty sure it should *not* look like it currently does. Yup, agreed on that. Semi-random other idea: What about introducing a new ->probe helper callback on connectors which would essentially entai all the things you want to change? I.e. what's currently done with ->detect plus connector status forcing, and the ->get_modes vs. edid override stuff? Step 1 would be to have a default implementation that does exactly the same thing. Plus reworking the hdp and poll helpers to prefer ->probe over just calling ->detect. Although for hpd I'm not sure whether using the same callback is a good idea really, at least in i915 we have a separate ->hpd callback for DP. And it needs it. And then we can try to figure out how to re-split responsibilities between drivers and helpers (and maybe try to stuff parts of it into specialized helpers, e.g. for DP). -Daniel
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)) + 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
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 ++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_probe_helper.c | 7 +++++ include/drm/drm_connector.h | 6 ++++ 3 files changed, 73 insertions(+)