Re: [PATCH v2] drm/bridge: analogix_dp: Ensure the panel is properly prepared/unprepared

[Daniel Vetter]

On Tue, Aug 09, 2016 at 10:35:57AM +0800, Yakir Yang wrote:
> + Archit
> 
> 
> On 08/09/2016 02:53 AM, Sean Paul wrote:
> > Instead of just preparing the panel on bind, actually prepare/unprepare
> > during modeset/disable. The panel must be prepared in order to read hpd
> > status and edid, so we need to keep state around the prepares in order
> > to ensure we don't accidentally turn the panel off at the wrong time.
> > 
> > Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> 
> Reviewed-by: Yakir Yang <ykk@xxxxxxxxxxxxxx>
> 
> And I also tested this patch on RK3399 Kevin board, panel works rightly, so:
> Tested-by: Yakir Yang <ykk@xxxxxxxxxxxxxx>
> 
> Also add Archit into CC list, guess this patch should go through his
> drm_bridge's tree.

drm_bridge stuff moved to -misc ;-)
-Daniel

> 
> Thanks,
> - Yakir
> 
> > ---
> > 
> > Changes in v2:
> >   - Added panel_is_modeset state/lock to avoid racing detect with modeset (marcheu)
> >   - Added prepare/unprepare in .get_modes (yakir)
> > 
> >   drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 101 ++++++++++++++++++---
> >   drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |   3 +
> >   2 files changed, 93 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > index 32715da..47c449a 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > @@ -923,11 +923,63 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
> >   	analogix_dp_start_video(dp);
> >   }
> > +/*
> > + * This function is a bit of a catch-all for panel preparation, hopefully
> > + * simplifying the logic of functions that need to prepare/unprepare the panel
> > + * below.
> > + *
> > + * If @prepare is true, this function will prepare the panel. Conversely, if it
> > + * is false, the panel will be unprepared.
> > + *
> > + * If @is_modeset_prepare is true, the function will disregard the current state
> > + * of the panel and either prepare/unprepare the panel based on @prepare. Once
> > + * it finishes, it will update dp->panel_is_modeset to reflect the current state
> > + * of the panel.
> > + */
> > +static int analogix_dp_prepare_panel(struct analogix_dp_device *dp,
> > +				     bool prepare, bool is_modeset_prepare)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!dp->plat_data->panel)
> > +		return 0;
> > +
> > +	mutex_lock(&dp->panel_lock);
> > +
> > +	/*
> > +	 * Exit early if this is a temporary prepare/unprepare and we're already
> > +	 * modeset (since we neither want to prepare twice or unprepare early).
> > +	 */
> > +	if (dp->panel_is_modeset && !is_modeset_prepare)
> > +		goto out;
> > +
> > +	if (prepare)
> > +		ret = drm_panel_prepare(dp->plat_data->panel);
> > +	else
> > +		ret = drm_panel_unprepare(dp->plat_data->panel);
> > +
> > +	if (ret)
> > +		goto out;
> > +
> > +	if (is_modeset_prepare)
> > +		dp->panel_is_modeset = prepare;
> > +
> > +out:
> > +	mutex_unlock(&dp->panel_lock);
> > +	return ret;
> > +}
> > +
> >   int analogix_dp_get_modes(struct drm_connector *connector)
> >   {
> >   	struct analogix_dp_device *dp = to_dp(connector);
> >   	struct edid *edid = (struct edid *)dp->edid;
> > -	int num_modes = 0;
> > +	int ret, num_modes = 0;
> > +
> > +	ret = analogix_dp_prepare_panel(dp, true, false);
> > +	if (ret) {
> > +		DRM_ERROR("Failed to prepare panel (%d)\n", ret);
> > +		return 0;
> > +	}
> >   	if (analogix_dp_handle_edid(dp) == 0) {
> >   		drm_mode_connector_update_edid_property(&dp->connector, edid);
> > @@ -940,6 +992,10 @@ int analogix_dp_get_modes(struct drm_connector *connector)
> >   	if (dp->plat_data->get_modes)
> >   		num_modes += dp->plat_data->get_modes(dp->plat_data, connector);
> > +	ret = analogix_dp_prepare_panel(dp, false, false);
> > +	if (ret)
> > +		DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
> > +
> >   	return num_modes;
> >   }
> > @@ -960,11 +1016,23 @@ enum drm_connector_status
> >   analogix_dp_detect(struct drm_connector *connector, bool force)
> >   {
> >   	struct analogix_dp_device *dp = to_dp(connector);
> > +	enum drm_connector_status status = connector_status_disconnected;
> > +	int ret;
> > -	if (analogix_dp_detect_hpd(dp))
> > +	ret = analogix_dp_prepare_panel(dp, true, false);
> > +	if (ret) {
> > +		DRM_ERROR("Failed to prepare panel (%d)\n", ret);
> >   		return connector_status_disconnected;
> > +	}
> > +
> > +	if (!analogix_dp_detect_hpd(dp))
> > +		status = connector_status_connected;
> > -	return connector_status_connected;
> > +	ret = analogix_dp_prepare_panel(dp, false, false);
> > +	if (ret)
> > +		DRM_ERROR("Failed to unprepare panel (%d)\n", ret);
> > +
> > +	return status;
> >   }
> >   static void analogix_dp_connector_destroy(struct drm_connector *connector)
> > @@ -1035,6 +1103,16 @@ static int analogix_dp_bridge_attach(struct drm_bridge *bridge)
> >   	return 0;
> >   }
> > +static void analogix_dp_bridge_pre_enable(struct drm_bridge *bridge)
> > +{
> > +	struct analogix_dp_device *dp = bridge->driver_private;
> > +	int ret;
> > +
> > +	ret = analogix_dp_prepare_panel(dp, true, true);
> > +	if (ret)
> > +		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> > +}
> > +
> >   static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
> >   {
> >   	struct analogix_dp_device *dp = bridge->driver_private;
> > @@ -1058,6 +1136,7 @@ static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
> >   static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
> >   {
> >   	struct analogix_dp_device *dp = bridge->driver_private;
> > +	int ret;
> >   	if (dp->dpms_mode != DRM_MODE_DPMS_ON)
> >   		return;
> > @@ -1077,6 +1156,10 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
> >   	pm_runtime_put_sync(dp->dev);
> > +	ret = analogix_dp_prepare_panel(dp, false, true);
> > +	if (ret)
> > +		DRM_ERROR("failed to setup the panel ret = %d\n", ret);
> > +
> >   	dp->dpms_mode = DRM_MODE_DPMS_OFF;
> >   }
> > @@ -1165,9 +1248,9 @@ static void analogix_dp_bridge_nop(struct drm_bridge *bridge)
> >   }
> >   static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
> > +	.pre_enable = analogix_dp_bridge_pre_enable,
> >   	.enable = analogix_dp_bridge_enable,
> >   	.disable = analogix_dp_bridge_disable,
> > -	.pre_enable = analogix_dp_bridge_nop,
> >   	.post_disable = analogix_dp_bridge_nop,
> >   	.mode_set = analogix_dp_bridge_mode_set,
> >   	.attach = analogix_dp_bridge_attach,
> > @@ -1254,6 +1337,9 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
> >   	dp->dev = &pdev->dev;
> >   	dp->dpms_mode = DRM_MODE_DPMS_OFF;
> > +	mutex_init(&dp->panel_lock);
> > +	dp->panel_is_modeset = false;
> > +
> >   	/*
> >   	 * platform dp driver need containor_of the plat_data to get
> >   	 * the driver private data, so we need to store the point of
> > @@ -1333,13 +1419,6 @@ int analogix_dp_bind(struct device *dev, struct drm_device *drm_dev,
> >   	phy_power_on(dp->phy);
> > -	if (dp->plat_data->panel) {
> > -		if (drm_panel_prepare(dp->plat_data->panel)) {
> > -			DRM_ERROR("failed to setup the panel\n");
> > -			return -EBUSY;
> > -		}
> > -	}
> > -
> >   	analogix_dp_init_dp(dp);
> >   	ret = devm_request_threaded_irq(&pdev->dev, dp->irq,
> > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > index b456380..43108d7 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> > @@ -178,6 +178,9 @@ struct analogix_dp_device {
> >   	bool                    force_hpd;
> >   	unsigned char           edid[EDID_BLOCK_LENGTH * 2];
> > +	struct mutex		panel_lock;
> > +	bool			panel_is_modeset;
> > +
> >   	struct analogix_dp_plat_data *plat_data;
> >   };
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch