Re: [PATCH 07/10] drm/i915: Create generic intel_panel for LVDS and eDP

Jesse Barnes
2012-10-19T17:03:53Z

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

> Create a generic struct intel_panel for sharing a data structure and code
> between eDP and LVDS panels. Add the new struct to intel_connector so that
> later on we can have generic EDID and mode reading functions with EDID
> caching that transparently fallback to fixed mode when EDID is not
> available.
>
> Add intel_panel as a dummy first, and move data (such as the mentioned
> fixed mode) to it in later patches.
>
> Based on earlier work by Chris Wilson <chris@chris-wilson.co.uk>
>
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 9 +++++++--
> drivers/gpu/drm/i915/intel_drv.h | 9 +++++++++
> drivers/gpu/drm/i915/intel_lvds.c | 2 ++
> drivers/gpu/drm/i915/intel_panel.c | 9 +++++++++
> 4 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 52cbee7..abbdfed 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2502,9 +2502,12 @@ static void
> intel_dp_destroy(struct drm_connector *connector)
> {
> struct drm_device *dev = connector->dev;
> + struct intel_connector *intel_connector = to_intel_connector(connector);
>
> - if (intel_dpd_is_edp(dev))
> + if (intel_dpd_is_edp(dev)) {
> intel_panel_destroy_backlight(dev);
> + intel_panel_fini(&intel_connector->panel);
> + }
>
> drm_sysfs_connector_remove(connector);
> drm_connector_cleanup(connector);
> @@ -2833,8 +2836,10 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>
> intel_encoder->hot_plug = intel_dp_hot_plug;
>
> - if (is_edp(intel_dp))
> + if (is_edp(intel_dp)) {
> + intel_panel_init(&intel_connector->panel);
> intel_panel_setup_backlight(connector);
> + }
>
> intel_dp_add_properties(intel_dp, connector);
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e5cdf3d..df00f21 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -163,6 +163,9 @@ struct intel_encoder {
> int crtc_mask;
> };
>
> +struct intel_panel {
> +};
> +
> struct intel_connector {
> struct drm_connector base;
> /*
> @@ -179,6 +182,9 @@ struct intel_connector {
> /* Reads out the current hw, returning true if the connector is enabled
> * and active (i.e. dpms ON state). */
> bool (*get_hw_state)(struct intel_connector *);
> +
> + /* Panel info for eDP and LVDS */
> + struct intel_panel panel;
> };
>
> struct intel_crtc {
> @@ -436,6 +442,9 @@ extern void intel_flush_display_plane(struct drm_i915_private *dev_priv,
> enum plane plane);
>
> /* intel_panel.c */
> +extern int intel_panel_init(struct intel_panel *panel);
> +extern void intel_panel_fini(struct intel_panel *panel);
> +
> extern void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
> struct drm_display_mode *adjusted_mode);
> extern void intel_pch_panel_fitting(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 0ed2c3b..db1b1b7 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -555,6 +555,7 @@ static void intel_lvds_destroy(struct drm_connector *connector)
> acpi_lid_notifier_unregister(&lvds_connector->lid_notifier);
>
> intel_panel_destroy_backlight(connector->dev);
> + intel_panel_fini(&lvds_connector->base.panel);
>
> drm_sysfs_connector_remove(connector);
> drm_connector_cleanup(connector);
> @@ -1107,6 +1108,7 @@ out:
> }
> drm_sysfs_connector_add(connector);
>
> + intel_panel_init(&intel_connector->panel);
> intel_panel_setup_backlight(connector);
>
> return true;
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index d9752a3..4c64ebc 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -464,3 +464,12 @@ void intel_panel_destroy_backlight(struct drm_device *dev)
> return;
> }
> #endif
> +
> +int intel_panel_init(struct intel_panel *panel)
> +{
> + return 0;
> +}
> +
> +void intel_panel_fini(struct intel_panel *panel)
> +{
> +}

Looks good. My only nitpick is that this could be any type of fixed
display, so panel may not be the best name. But practically speaking I
guess MIPI, eDP, LVDS, or even fixed HDMI or DVI displays will be
panels so no biggie.

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

--
Jesse Barnes, Intel Open Source Technology Center