Re: [PATCH 10/10] drm/i915: Move cached EDID to intel_connector

Jesse Barnes
2012-10-19T17:12:13Z

On Fri, 19 Oct 2012 14:51:52 +0300
Jani Nikula <jani.nikula@intel.com> wrote:

> Move the cached EDID from intel_dp and intel_lvds_connector to
> intel_connector. Unify cached EDID handling for LVDS and eDP, in
> preparation for adding more generic EDID caching later.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 61 +++++++++++++++++++++----------------
> drivers/gpu/drm/i915/intel_drv.h | 5 +--
> drivers/gpu/drm/i915/intel_lvds.c | 34 ++++++++++++++-------
> 3 files changed, 60 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8c72477..5b0e4cd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2323,44 +2323,45 @@ g4x_dp_detect(struct intel_dp *intel_dp)
> static struct edid *
> intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
> {
> - struct intel_dp *intel_dp = intel_attached_dp(connector);
> - struct edid *edid;
> - int size;
> + struct intel_connector *intel_connector = to_intel_connector(connector);
>
> - if (is_edp(intel_dp)) {
> - if (!intel_dp->edid)
> + /* use cached edid if we have one */
> + if (intel_connector->edid) {
> + struct edid *edid;
> + int size;
> +
> + /* invalid edid */
> + if (IS_ERR(intel_connector->edid))
> return NULL;
>
> - size = (intel_dp->edid->extensions + 1) * EDID_LENGTH;
> + size = (intel_connector->edid->extensions + 1) * EDID_LENGTH;
> edid = kmalloc(size, GFP_KERNEL);
> if (!edid)
> return NULL;
>
> - memcpy(edid, intel_dp->edid, size);
> + memcpy(edid, intel_connector->edid, size);
> return edid;
> }
>
> - edid = drm_get_edid(connector, adapter);
> - return edid;
> + return drm_get_edid(connector, adapter);
> }
>
> static int
> intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *adapter)
> {
> - struct intel_dp *intel_dp = intel_attached_dp(connector);
> - int ret;
> + struct intel_connector *intel_connector = to_intel_connector(connector);
>
> - if (is_edp(intel_dp)) {
> - drm_mode_connector_update_edid_property(connector,
> - intel_dp->edid);
> - ret = drm_add_edid_modes(connector, intel_dp->edid);
> - drm_edid_to_eld(connector,
> - intel_dp->edid);
> - return intel_dp->edid_mode_count;
> + /* use cached edid if we have one */
> + if (intel_connector->edid) {
> + /* invalid edid */
> + if (IS_ERR(intel_connector->edid))
> + return 0;
> +
> + return intel_connector_update_modes(connector,
> + intel_connector->edid);
> }
>
> - ret = intel_ddc_get_modes(connector, adapter);
> - return ret;
> + return intel_ddc_get_modes(connector, adapter);
> }
>
>
> @@ -2511,6 +2512,9 @@ intel_dp_destroy(struct drm_connector *connector)
> struct drm_device *dev = connector->dev;
> struct intel_connector *intel_connector = to_intel_connector(connector);
>
> + if (!IS_ERR_OR_NULL(intel_connector->edid))
> + kfree(intel_connector->edid);
> +
> if (intel_dpd_is_edp(dev)) {
> intel_panel_destroy_backlight(dev);
> intel_panel_fini(&intel_connector->panel);
> @@ -2528,7 +2532,6 @@ static void intel_dp_encoder_destroy(struct drm_encoder *encoder)
> i2c_del_adapter(&intel_dp->adapter);
> drm_encoder_cleanup(encoder);
> if (is_edp(intel_dp)) {
> - kfree(intel_dp->edid);
> cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> ironlake_panel_vdd_off_sync(intel_dp);
> }
> @@ -2815,13 +2818,17 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
> ironlake_edp_panel_vdd_on(intel_dp);
> edid = drm_get_edid(connector, &intel_dp->adapter);
> if (edid) {
> - drm_mode_connector_update_edid_property(connector,
> - edid);
> - intel_dp->edid_mode_count =
> - drm_add_edid_modes(connector, edid);
> - drm_edid_to_eld(connector, edid);
> - intel_dp->edid = edid;
> + if (drm_add_edid_modes(connector, edid)) {
> + drm_mode_connector_update_edid_property(connector, edid);
> + drm_edid_to_eld(connector, edid);
> + } else {
> + kfree(edid);
> + edid = ERR_PTR(-EINVAL);
> + }
> + } else {
> + edid = ERR_PTR(-ENOENT);
> }
> + intel_connector->edid = edid;
>
> /* prefer fixed mode from EDID if available */
> list_for_each_entry(scan, &connector->probed_modes, head) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ae0467a..5fa0af8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -186,6 +186,9 @@ struct intel_connector {
>
> /* Panel info for eDP and LVDS */
> struct intel_panel panel;
> +
> + /* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */
> + struct edid *edid;
> };
>
> struct intel_crtc {
> @@ -367,8 +370,6 @@ struct intel_dp {
> int backlight_off_delay;
> struct delayed_work panel_vdd_work;
> bool want_panel_vdd;
> - struct edid *edid; /* cached EDID for eDP */
> - int edid_mode_count;
> struct intel_connector *attached_connector;
> };
>
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index ced6947..efa62ac 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -45,7 +45,6 @@ struct intel_lvds_connector {
> struct intel_connector base;
>
> struct notifier_block lid_notifier;
> - struct edid *edid;
> int fitting_mode;
> };
>
> @@ -461,8 +460,14 @@ static int intel_lvds_get_modes(struct drm_connector *connector)
> struct drm_device *dev = connector->dev;
> struct drm_display_mode *mode;
>
> - if (lvds_connector->edid)
> - return drm_add_edid_modes(connector, lvds_connector->edid);
> + /* use cached edid if we have one */
> + if (lvds_connector->base.edid) {
> + /* invalid edid */
> + if (IS_ERR(lvds_connector->base.edid))
> + return 0;
> +
> + return drm_add_edid_modes(connector, lvds_connector->base.edid);
> + }
>
> mode = drm_mode_duplicate(dev, lvds_connector->base.panel.fixed_mode);
> if (mode == NULL)
> @@ -554,6 +559,9 @@ static void intel_lvds_destroy(struct drm_connector *connector)
> if (lvds_connector->lid_notifier.notifier_call)
> acpi_lid_notifier_unregister(&lvds_connector->lid_notifier);
>
> + if (!IS_ERR_OR_NULL(lvds_connector->base.edid))
> + kfree(lvds_connector->base.edid);
> +
> intel_panel_destroy_backlight(connector->dev);
> intel_panel_fini(&lvds_connector->base.panel);
>
> @@ -923,6 +931,7 @@ bool intel_lvds_init(struct drm_device *dev)
> struct drm_encoder *encoder;
> struct drm_display_mode *scan; /* *modes, *bios_mode; */
> struct drm_display_mode *fixed_mode = NULL;
> + struct edid *edid;
> struct drm_crtc *crtc;
> u32 lvds;
> int pipe;
> @@ -1022,18 +1031,21 @@ bool intel_lvds_init(struct drm_device *dev)
> * Attempt to get the fixed panel mode from DDC. Assume that the
> * preferred mode is the right one.
> */
> - lvds_connector->edid = drm_get_edid(connector,
> - intel_gmbus_get_adapter(dev_priv, pin));
> - if (lvds_connector->edid) {
> - if (drm_add_edid_modes(connector, lvds_connector->edid)) {
> + edid = drm_get_edid(connector, intel_gmbus_get_adapter(dev_priv, pin));
> + if (edid) {
> + if (drm_add_edid_modes(connector, edid)) {
> drm_mode_connector_update_edid_property(connector,
> - lvds_connector->edid);
> + edid);
> } else {
> - kfree(lvds_connector->edid);
> - lvds_connector->edid = NULL;
> + kfree(edid);
> + edid = ERR_PTR(-EINVAL);
> }
> + } else {
> + edid = ERR_PTR(-ENOENT);
> }
> - if (!lvds_connector->edid) {
> + lvds_connector->base.edid = edid;
> +
> + if (IS_ERR_OR_NULL(edid)) {
> /* Didn't get an EDID, so
> * Set wide sync ranges so we get all modes
> * handed to valid_mode for checking

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Looks like 6-10 should also just go upstream, looks like good stuff.

--
Jesse Barnes, Intel Open Source Technology Center