linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: Jernej Skrabec <jernej.skrabec@siol.net>,
	drinkcat@chromium.org, Neil Armstrong <narmstrong@baylibre.com>,
	David Airlie <airlied@linux.ie>, Jonas Karlman <jonas@kwiboo.se>,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	Andrzej Hajda <a.hajda@samsung.com>,
	Boris Brezillon <boris.brezillon@collabora.com>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	hsinyi@chromium.org, matthias.bgg@gmail.com,
	Collabora Kernel ML <kernel@collabora.com>
Subject: Re: [PATCH 3/3] drm/bridge: ps8640: Rework power state handling
Date: Thu, 25 Jun 2020 11:21:44 +0200	[thread overview]
Message-ID: <20b2f097-d789-3f5a-cd7e-3701669f43cb@collabora.com> (raw)
In-Reply-To: <20200624070738.GA1807736@ravnborg.org>

Hi Sam,

On 24/6/20 9:07, Sam Ravnborg wrote:
> Hi Enric.
> 
> On Tue, Jun 23, 2020 at 05:16:43PM +0200, Enric Balletbo i Serra wrote:
>> Hi Sam,
>>
>> Many thanks for your feedback. See my answers below.
>>
>> On 20/6/20 23:42, Sam Ravnborg wrote:
>>> Hi Enric.
>>>
>>> On Mon, Jun 15, 2020 at 10:53:20PM +0200, Enric Balletbo i Serra wrote:
>>>> The get_edid() callback can be triggered anytime by an ioctl, i.e
>>>>
>>>>   drm_mode_getconnector (ioctl)
>>>>     -> drm_helper_probe_single_connector_modes
>>>>        -> drm_bridge_connector_get_modes
>>>>           -> ps8640_bridge_get_edid
>>>>
>>>> Actually if the bridge pre_enable() function was not called before
>>>> get_edid(), the driver will not be able to get the EDID properly and
>>>> display will not work until a second get_edid() call is issued and if
>>>> pre_enable() is called before.
>>> Is it correct to fix this in the driver?
>>> Why not just fail and tell user-sapce to try again later?
>>> (Dunno what error-code to use - there must be one).
>>>
>>
>> My undestanding, I might be wrong, is that userspace should don't know which
>> bits, regulators, etc, are needed to get the EDID with an ioctl. Is the kernel
>> that should make sure that all is set properly (the required regulators, etc)
>> when userspace wants to read the EDID.
> 
> The idea I suggest is that the kernel side should just error
> out and let user-space retry later.

I see, I got it.

> So we avoid all the extra logic in the kernel because userspace wants
> info before the HW is ready.
> And userspace shall anyway deal with the ioctl that fails.
> 

My doubt here is, at which point the kernel driver should leave the hardware in
a state that getting the EDID should work? After the bridge pre_enable() or
enable() calls? After has been proved? AFAIK the bridge pre_enable() and
enable() calls are only about enable the stream out and in most cases you can
get the EDID without having to call pre_enable() and enable() before.

Thanks,
 Enric


> 	Sam
> 
>>
>>> Then we avoid complicating drivers fro somethign we really should
>>> fix in user-space.
>>>
>>>> The side effect of this, for example, is
>>>> that you see anything when `Frecon` starts, neither the splash screen,
>>> that you do not see ...
>>>
>>> (Otherwise I do not parse the above).
>>>
>>>> until the graphical session manager starts.
>>>>
>>>> To fix this we need to make sure that all we need is enabled before
>>>> reading the EDID. This means the following:
>>>>
>>>> 1. If get_edid() is called before having the device powered we need to
>>>>    power on the device. In such case, the driver will power off again the
>>>>    device.
>>>>
>>>> 2. If get_edid() is called after having the device powered, all should
>>>>    just work. We added a powered flag in order to avoid recurrent calls
>>>>    to ps8640_bridge_poweron() and unneeded delays.
>>>>
>>>> 3. This seems to be specific for this device, but we need to make sure
>>>>    the panel is powered on before do a power on cycle on this device.
>>>>    Otherwise the device fails to retrieve the EDID.
>>> Step 3. looks like an ugly hack too....
>>>
>>
>> It is, but I don't have enough hardware details to be able to answer why we need
>> to do this. What is well tested is that, if I don't power the panel before
>> powering on the bridge, it doesn't get a proper EDID. Seems that when the bridge
>> goes up, the firmware tries to read the EDID and caches somehow. Well not sure.
>>
>>>>
>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>> ---
>>>>
>>>>  drivers/gpu/drm/bridge/parade-ps8640.c | 79 ++++++++++++++++++++++++--
>>>>  1 file changed, 73 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
>>>> index 9f7b7a9c53c52..ca651480891df 100644
>>>> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
>>>> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
>>>> @@ -65,6 +65,7 @@ struct ps8640 {
>>>>  	struct regulator_bulk_data supplies[2];
>>>>  	struct gpio_desc *gpio_reset;
>>>>  	struct gpio_desc *gpio_powerdown;
>>>> +	bool powered;
>>>>  };
>>>>  
>>>>  static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
>>>> @@ -91,13 +92,25 @@ static int ps8640_bridge_vdo_control(struct ps8640 *ps_bridge,
>>>>  	return 0;
>>>>  }
>>>>  
>>>> -static void ps8640_pre_enable(struct drm_bridge *bridge)
>>>> +static void ps8640_bridge_poweron(struct ps8640 *ps_bridge)
>>>>  {
>>>> -	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>>>  	struct i2c_client *client = ps_bridge->page[PAGE2_TOP_CNTL];
>>>> +	struct drm_bridge *panel;
>>>>  	unsigned long timeout;
>>>>  	int ret, status;
>>>>  
>>>> +	if (ps_bridge->powered)
>>>> +		return;
>>>> +
>>>> +	/*
>>>> +	 * That seems to be specific to this chip, and a weird behaviour, but
>>>> +	 * we need to call drm_panel_prepare before issuing a poweron cycle. If
>>>> +	 * we don't do this, the chip is not able to read properly the EDID.
>>>> +	 */
>>>> +	panel = ps_bridge->panel_bridge;
>>>> +	if (panel->funcs && panel->funcs->pre_enable)
>>>> +		panel->funcs->pre_enable(panel);
>>>> +
>>>>  	ret = regulator_bulk_enable(ARRAY_SIZE(ps_bridge->supplies),
>>>>  				    ps_bridge->supplies);
>>>>  	if (ret < 0) {
>>>> @@ -164,6 +177,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>>>>  		goto err_regulators_disable;
>>>>  	}
>>>>  
>>>> +	ps_bridge->powered = true;
>>>> +
>>>>  	return;
>>>>  
>>>>  err_regulators_disable:
>>>> @@ -171,12 +186,13 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>>>>  			       ps_bridge->supplies);
>>>>  }
>>>>  
>>>> -static void ps8640_post_disable(struct drm_bridge *bridge)
>>>> +static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
>>>>  {
>>>> -	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>>> +	struct drm_bridge *panel;
>>>>  	int ret;
>>>>  
>>>> -	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
>>>> +	if (!ps_bridge->powered)
>>>> +		return;
>>>>  
>>>>  	gpiod_set_value(ps_bridge->gpio_reset, 1);
>>>>  	gpiod_set_value(ps_bridge->gpio_powerdown, 1);
>>>> @@ -184,6 +200,32 @@ static void ps8640_post_disable(struct drm_bridge *bridge)
>>>>  				     ps_bridge->supplies);
>>>>  	if (ret < 0)
>>>>  		DRM_ERROR("cannot disable regulators %d\n", ret);
>>>> +
>>>> +	panel = ps_bridge->panel_bridge;
>>>> +	if (panel->funcs && panel->funcs->post_disable)
>>>> +		panel->funcs->post_disable(panel);
>>>> +
>>>> +	ps_bridge->powered = false;
>>>> +}
>>>> +
>>>> +static void ps8640_pre_enable(struct drm_bridge *bridge)
>>>> +{
>>>> +	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>>> +	int ret;
>>>> +
>>>> +	ps8640_bridge_poweron(ps_bridge);
>>>> +
>>>> +	ret = ps8640_bridge_vdo_control(ps_bridge, DISABLE);
>>>> +	if (ret < 0)
>>>> +		ps8640_bridge_poweroff(ps_bridge);
>>>> +}
>>>> +
>>>> +static void ps8640_post_disable(struct drm_bridge *bridge)
>>>> +{
>>>> +	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>>> +
>>>> +	ps8640_bridge_vdo_control(ps_bridge, DISABLE);
>>>> +	ps8640_bridge_poweroff(ps_bridge);
>>>>  }
>>>>  
>>>>  static int ps8640_bridge_attach(struct drm_bridge *bridge,
>>>> @@ -249,9 +291,34 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>>>>  					   struct drm_connector *connector)
>>>>  {
>>>>  	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>>>> +	bool poweroff = !ps_bridge->powered;
>>>> +	struct edid *edid;
>>>> +
>>>> +	/*
>>>> +	 * When we end calling get_edid() triggered by an ioctl, i.e
>>>> +	 *
>>>> +	 *   drm_mode_getconnector (ioctl)
>>>> +	 *     -> drm_helper_probe_single_connector_modes
>>>> +	 *        -> drm_bridge_connector_get_modes
>>>> +	 *           -> ps8640_bridge_get_edid
>>>> +	 *
>>>> +	 * We need to make sure that what we need is enabled before reading
>>>> +	 * EDID, for this chip, we need to do a full poweron, otherwise it will
>>>> +	 * fail.
>>>> +	 */
>>>> +	ps8640_bridge_poweron(ps_bridge);
>>>>  
>>>> -	return drm_get_edid(connector,
>>>> +	edid = drm_get_edid(connector,
>>>>  			    ps_bridge->page[PAGE0_DP_CNTL]->adapter);
>>>> +
>>>> +	/*
>>>> +	 * If we call the get_edid() function without having enabled the chip
>>>> +	 * before, return the chip to its original power state.
>>>> +	 */
>>>> +	if (poweroff)
>>>> +		ps8640_bridge_poweroff(ps_bridge);
>>>> +
>>>> +	return edid;
>>>>  }
>>>>  
>>>>  static const struct drm_bridge_funcs ps8640_bridge_funcs = {
>>>> -- 
>>>> 2.27.0
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

  reply	other threads:[~2020-06-25  9:21 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-15 20:53 [PATCH 0/3] drm/bridge: ps8640: Make sure all needed is powered to get the EDID Enric Balletbo i Serra
2020-06-15 20:53 ` [PATCH 1/3] drm/bridge: ps8640: Return an error for incorrect attach flags Enric Balletbo i Serra
2020-08-22  9:22   ` Sam Ravnborg
2020-06-15 20:53 ` [PATCH 2/3] drm/bridge: ps8640: Print an error if VDO control fails Enric Balletbo i Serra
2020-08-22  9:23   ` Sam Ravnborg
2020-06-15 20:53 ` [PATCH 3/3] drm/bridge: ps8640: Rework power state handling Enric Balletbo i Serra
2020-06-20 21:42   ` Sam Ravnborg
2020-06-23 15:16     ` Enric Balletbo i Serra
2020-06-24  7:07       ` Sam Ravnborg
2020-06-25  9:21         ` Enric Balletbo i Serra [this message]
2020-08-21 11:14           ` Enric Balletbo i Serra
2020-08-21 11:38             ` Sam Ravnborg
2020-08-22  9:39               ` Sam Ravnborg
2020-08-22  9:38   ` Sam Ravnborg

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20b2f097-d789-3f5a-cd7e-3701669f43cb@collabora.com \
    --to=enric.balletbo@collabora.com \
    --cc=Laurent.pinchart@ideasonboard.com \
    --cc=a.hajda@samsung.com \
    --cc=airlied@linux.ie \
    --cc=boris.brezillon@collabora.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=drinkcat@chromium.org \
    --cc=hsinyi@chromium.org \
    --cc=jernej.skrabec@siol.net \
    --cc=jonas@kwiboo.se \
    --cc=kernel@collabora.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=matthias.bgg@gmail.com \
    --cc=narmstrong@baylibre.com \
    --cc=sam@ravnborg.org \
    --subject='Re: [PATCH 3/3] drm/bridge: ps8640: Rework power state handling' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).