lkml.org 
[lkml]   [2019]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 2/2] drm/rockchip: dw_hdmi: Handle suspend/resume
    On Thu, Jun 06, 2019 at 03:58:21PM -0700, Doug Anderson wrote:
    > Hi,
    >
    > On Thu, Jun 6, 2019 at 9:42 AM Sean Paul <sean@poorly.run> wrote:
    > >
    > > On Tue, Jun 04, 2019 at 01:42:07PM -0700, Douglas Anderson wrote:
    > > > On Rockchip rk3288-based Chromebooks when you do a suspend/resume
    > > > cycle:
    > > >
    > > > 1. You lose the ability to detect an HDMI device being plugged in.
    > > >
    > > > 2. If you're using the i2c bus built in to dw_hdmi then it stops
    > > > working.
    > > >
    > > > Let's call the core dw-hdmi's suspend/resume functions to restore
    > > > things.
    > > >
    > > > NOTE: in downstream Chrome OS (based on kernel 3.14) we used the
    > > > "late/early" versions of suspend/resume because we found that the VOP
    > > > was sometimes resuming before dw_hdmi and then calling into us before
    > > > we were fully resumed. For now I have gone back to the normal
    > > > suspend/resume because I can't reproduce the problems.
    > > >
    > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
    > > > ---
    > > >
    > > > Changes in v3:
    > > > - dw_hdmi_resume() is now a void function (Laurent)
    > > >
    > > > Changes in v2:
    > > > - Add forgotten static (Laurent)
    > > > - No empty stub for suspend (Laurent)
    > > >
    > > > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 14 ++++++++++++++
    > > > 1 file changed, 14 insertions(+)
    > > >
    > > > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
    > > > index 4cdc9f86c2e5..7bb0f922b303 100644
    > > > --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
    > > > +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
    > > > @@ -542,11 +542,25 @@ static int dw_hdmi_rockchip_remove(struct platform_device *pdev)
    > > > return 0;
    > > > }
    > > >
    > > > +static int __maybe_unused dw_hdmi_rockchip_resume(struct device *dev)
    > > > +{
    > > > + struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
    > > > +
    > > > + dw_hdmi_resume(hdmi->hdmi);
    > >
    > > The rockchip driver is already using the atomic suspend/resume helpers (via the
    > > modeset helpers). Would you be able to accomplish the same thing by just moving
    > > this call into the encoder enable callback?
    > >
    > > .enable is called on resume via the atomic commit framework, so everything is
    > > ordered properly. Of course, this would reset the dw_hdmi bridge on each enable,
    > > but I don't think that would be a problem?
    >
    > I tried and it sorta kinda half worked, but...
    >
    > 1. One of the problems solved by this patch is making "hot plug
    > detect" work after suspend / resume. AKA: if you have nothing plugged
    > in to the HDMI port and then suspend/resume you need to be able to
    > detect when something is plugged in. When nothing is plugged in then
    > the ".enable" isn't called at resume time.
    >

    Ahh, ok. So we've hit this with other bridges/dongles as well, and yeah the
    solution is to keep the bridge powered up enough to detect hotplug, so you would
    need to do some work in .resume

    Usually there's a second stage of enable where you power things on more fully
    and that is done in .enable

    > 2. I'm not so convinced about the whole ordering being correct.
    > Unfortunately on my system (Chrome OS running the chromeos-4.19
    > kernel) we end up getting an i2c transfer before the ".enable" is
    > called. I put a dump_stack() in the i2c transfer:
    >
    > [ 42.212516] CPU: 0 PID: 1479 Comm: DrmThread Tainted: G C
    > 4.19.47 #60
    > [ 42.221182] Hardware name: Rockchip (Device Tree)
    > [ 42.226449] [<c0211a64>] (unwind_backtrace) from [<c020cf0c>]
    > (show_stack+0x20/0x24)
    > [ 42.235114] [<c020cf0c>] (show_stack) from [<c0a1b8d4>]
    > (dump_stack+0x84/0xa4)
    > [ 42.243195] [<c0a1b8d4>] (dump_stack) from [<c067d7c4>]
    > (dw_hdmi_i2c_wait+0x6c/0xa8)
    > [ 42.251858] [<c067d7c4>] (dw_hdmi_i2c_wait) from [<c067d9a8>]
    > (dw_hdmi_i2c_xfer+0x1a8/0x30c)
    > [ 42.261298] [<c067d9a8>] (dw_hdmi_i2c_xfer) from [<c0798704>]
    > (__i2c_transfer+0x3a8/0x5d8)
    > [ 42.270543] [<c0798704>] (__i2c_transfer) from [<c07989c8>]
    > (i2c_transfer+0x94/0xc4)
    > [ 42.279204] [<c07989c8>] (i2c_transfer) from [<c064e6b0>]
    > (drm_do_probe_ddc_edid+0xbc/0x11c)
    > [ 42.288642] [<c064e6b0>] (drm_do_probe_ddc_edid) from [<c064e744>]
    > (drm_probe_ddc+0x34/0x5c)
    > [ 42.298081] [<c064e744>] (drm_probe_ddc) from [<c0651b98>]
    > (drm_get_edid+0x60/0x2e0)
    > [ 42.306743] [<c0651b98>] (drm_get_edid) from [<c067d710>]
    > (dw_hdmi_connector_get_modes+0x30/0x78)
    > [ 42.316669] [<c067d710>] (dw_hdmi_connector_get_modes) from
    > [<c0634f38>] (drm_helper_probe_single_connector_modes+0x218/0x5c0)
    > [ 42.329413] [<c0634f38>] (drm_helper_probe_single_connector_modes)
    > from [<c065b38c>] (drm_mode_getconnector+0x144/0x418)
    > [ 42.341573] [<c065b38c>] (drm_mode_getconnector) from [<c0646844>]
    > (drm_ioctl_kernel+0xa0/0xf0)
    > [ 42.351303] [<c0646844>] (drm_ioctl_kernel) from [<c0646d34>]
    > (drm_ioctl+0x32c/0x3c0)
    > [ 42.360063] [<c0646d34>] (drm_ioctl) from [<c03ed0cc>] (vfs_ioctl+0x28/0x44)
    > [ 42.367946] [<c03ed0cc>] (vfs_ioctl) from [<c03edee8>]
    > (do_vfs_ioctl+0x718/0x8b0)
    > [ 42.376315] [<c03edee8>] (do_vfs_ioctl) from [<c03ee0dc>]
    > (ksys_ioctl+0x5c/0x84)
    > [ 42.384587] [<c03ee0dc>] (ksys_ioctl) from [<c03ee11c>] (sys_ioctl+0x18/0x1c)
    > [ 42.392570] [<c03ee11c>] (sys_ioctl) from [<c02011d4>]
    > (__sys_trace_return+0x0/0x10)
    >
    > ...I see several transfers fail and then finally a few seconds later
    > finally see the .enable call:

    This is usually solved by wrapping the code in detect() with an enable/disable
    pair to turn on enough circuitry to do the i2c writes for edid read.

    >
    > [ 44.021501] DOUG: dw_hdmi_rockchip_encoder_enable start
    > [ 44.027792] DOUG: dw_hdmi_rockchip_encoder_enable end
    >
    > I can gather more info if it's useful.
    >
    > ===
    >
    > ...any chance we can keep the patch as-is, or do you have ideas of how
    > to solve the above problems?

    Yeah, given the context I think this is Ok to apply as-is. Maybe we could
    shave out some of the work done in resume and move it to a helper called in
    enable/detect, but I don't think it's necessary to hold up getting things
    working.

    Thanks for the detailed explanation (as always :)

    Reviewed-by: Sean Paul <sean@poorly.run>


    >
    >
    > Thanks!
    >
    > -Doug

    --
    Sean Paul, Software Engineer, Google / Chromium OS
    > [ 42.351303] [<c0646844>] (drm_ioctl_kernel) from [<c0646d34>]
    > (drm_ioctl+0x32c/0x3c0)
    > [ 42.360063] [<c0646d34>] (drm_ioctl) from [<c03ed0cc>] (vfs_ioctl+0x28/0x44)
    > [ 42.367946] [<c03ed0cc>] (vfs_ioctl) from [<c03edee8>]
    > (do_vfs_ioctl+0x718/0x8b0)
    > [ 42.376315] [<c03edee8>] (do_vfs_ioctl) from [<c03ee0dc>]
    > (ksys_ioctl+0x5c/0x84)
    > [ 42.384587] [<c03ee0dc>] (ksys_ioctl) from [<c03ee11c>] (sys_ioctl+0x18/0x1c)
    > [ 42.392570] [<c03ee11c>] (sys_ioctl) from [<c02011d4>]
    > (__sys_trace_return+0x0/0x10)
    >
    > ...I see several transfers fail and then finally a few seconds later
    > finally see the .enable call:
    >
    > [ 44.021501] DOUG: dw_hdmi_rockchip_encoder_enable start
    > [ 44.027792] DOUG: dw_hdmi_rockchip_encoder_enable end
    >
    > I can gather more info if it's useful.
    >
    > ===
    >
    > ...any chance we can keep the patch as-is, or do you have ideas of how
    > to solve the above problems?
    >
    >
    > Thanks!
    >
    > -Doug

    --
    Sean Paul, Software Engineer, Google / Chromium OS

    \
     
     \ /
      Last update: 2019-06-07 20:07    [W:4.779 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site