lkml.org 
[lkml]   [2016]   [Jul]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable
    On Wed, Jul 27, 2016 at 11:28 PM, Yakir Yang <ykk@rock-chips.com> wrote:
    > Sean,
    >
    > Oops, sorry about miss your suggest :(
    >
    >
    > On 07/22/2016 11:03 PM, Sean Paul wrote:
    >>
    >> On Thu, Jul 21, 2016 at 9:00 PM, Yakir Yang <ykk@rock-chips.com> wrote:
    >>>
    >>> Sean,
    >>>
    >>> Thanks for your fast respond :-)
    >>>
    >>> But this patch is not the latest one, I have upgraded this to "v1.1"
    >>> version
    >>> to fix the eDP can't be disabled problem:
    >>> [PATCH v1.1 2/2] drm/bridge: analogix_dp: turn off the panel when
    >>> eDP
    >>> need to disable
    >>> Changes in v1.1: - unprepare the panel at the end of
    >>> bridge->disable() function
    >>>
    >>> In spite of this, I would take your comments with my "v1.1" patch.
    >>>
    >>> On 07/21/2016 10:28 PM, Sean Paul wrote:
    >>>
    >>> On Thu, Jul 21, 2016 at 9:14 AM, Yakir Yang <ykk@rock-chips.com> wrote:
    >>>
    >>> Some panels (like Sharp LQ123P1JX31) need to be turn off when eDP
    >>> controller stop to send valid video signal, otherwhise panel would
    >>> go burn in, and keep flicker and flicker.
    >>>
    >>> So it's better to turn off the panel when eDP need to disable, and
    >>> we need to turn on the panel in connector->detect() callback, so
    >>> that driver would detect panel status rightly.
    >>>
    >>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
    >>> ---
    >>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 18
    >>> ++++++++++--------
    >>> 1 file changed, 10 insertions(+), 8 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..ea059b3 100644
    >>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
    >>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
    >>> @@ -961,6 +961,14 @@ analogix_dp_detect(struct drm_connector *connector,
    >>> bool force)
    >>> {
    >>> struct analogix_dp_device *dp = to_dp(connector);
    >>>
    >>> + /*
    >>> + * Panle would prepare for several times here, but don't worry it
    >>>
    >>> s/Panle/Panel/
    >>>
    >>>
    >>> Done
    >>>
    >>> + * would only enable the hardware at the first prepare time.
    >>>
    >>> Errr, this shouldn't go in detect. How about putting this in
    >>> bridge_enable instead?
    >>>
    >>>
    >>> Nope, if we put this in bridge_enable, then eDP would never be enabled.
    >>> Here're the calling flow.
    >>>
    >>> --> analogix_dp_probe
    >>> --> analogix_dp_bind # we still haven't prepared the panel here, that
    >>> means panel have been powered up
    >>> --> analogix_dp_detect # Oops, losing panel valid hotplug signal,
    >>> cause
    >>> panel have been powered up
    >>> --> ** Keep detecting **
    >>
    >>
    >> Yeah, after playing around with the patch yesterday I discovered this
    >> for myself. I still don't think detect() should be changing hardware
    >> state. Perhaps you could add a matching unprepare() after you attempt
    >> to detect the panel?
    >
    >
    > Hmm, I don't understand the meaning of "add a matching unprepare()".
    >
    > I didn't see there is an unprepare() callback in drm_crtc_helper.h, would
    > you like to share some simple code :-D
    >

    In other words, make sure the hardware is in the same state at the
    beginning and end of detect(). You shouldn't have the panel in a
    different state when you exit detect().

    Sean



    > Thanks,
    > - Yakir
    >
    >
    >> Sean
    >>
    >>> + */
    >>> + if (dp->plat_data->panel)
    >>> + if (drm_panel_prepare(dp->plat_data->panel))
    >>>
    >>> Personally, I don't like doing work in a conditional since you're
    >>> throwing the return code away. Could you break this out into:
    >>>
    >>> ret = drm_panel_prepare(dp->plat_data->panel);
    >>> if (ret)
    >>> DRM_ERROR("failed to setup the panel ret=%d\n", ret);
    >>>
    >>>
    >>> Okay
    >>>
    >>>
    >>> + DRM_ERROR("failed to setup the panel\n");
    >>> +
    >>> if (analogix_dp_detect_hpd(dp))
    >>> return connector_status_disconnected;
    >>>
    >>> @@ -1063,7 +1071,8 @@ static void analogix_dp_bridge_disable(struct
    >>> drm_bridge *bridge)
    >>> return;
    >>>
    >>> if (dp->plat_data->panel) {
    >>> - if (drm_panel_disable(dp->plat_data->panel)) {
    >>> + if (drm_panel_disable(dp->plat_data->panel) ||
    >>> + drm_panel_unprepare(dp->plat_data->panel)) {
    >>>
    >>> Same comment here, please break this out into separate statements for
    >>> better readability/logging.
    >>>
    >>>
    >>> Okay,
    >>>
    >>>
    >>> Thanks,
    >>> - Yakir
    >>>
    >>>
    >>> DRM_ERROR("failed to disable the panel\n");
    >>> return;
    >>> }
    >>> @@ -1333,13 +1342,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,
    >>> --
    >>> 1.9.1
    >>>
    >>>
    >>>
    >>>
    >>>
    >>
    >>
    >
    >
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
    > in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html

    \
     
     \ /
      Last update: 2016-07-28 16:41    [W:5.178 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site