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

[Yakir Yang]

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 at 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 at 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 **

>> +        */
>> +       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
>>
>>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20160722/5c4a6f5c/attachment.html>