Re: [PATCH v1 2/2] drm/bridge: analogix_dp: turn off the panel when eDP need to disable

[Sean Paul]

On Wed, Jul 27, 2016 at 11:28 PM, Yakir Yang <ykk@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>>> ---
>>>   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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html