Re: [PATCH] drm/analogix_dp: Ensure the panel is properly prepared/unprepared

[Sean Paul]

On Mon, Aug 1, 2016 at 4:20 PM, Sean Paul <seanpaul@xxxxxxxxxxxx> wrote:
> On Sun, Jul 31, 2016 at 11:27 PM, Yakir Yang <ykk@xxxxxxxxxxxxxx> wrote:
>> Sean,
>>
>> On 07/30/2016 03:16 AM, Sean Paul wrote:
>>>
>>> Instead of just preparing the panel on bind, actually prepare/unprepare
>>> during modeset/disable. The panel must be prepared in order to read hpd
>>> status, so we need to refcount the prepares in order to ensure we don't
>>> accidentally turn the panel off at the wrong time.
>>>
>>> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
>>> ---
>>>
>>>
>>> Hi Yakir,
>>> This is what I was talking about upthread. I've tested it and it seems to
>>> be working.
>>>
>>> What do you think?
>>
>>
>> Thanks for your patch, and it works. But I have introduced two questions,
>> and I haven't found a way to fixed them.
>>
>>
>>> Sean
>>>
>>>
>>>
>>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 48
>>> +++++++++++++++++-----
>>>   1 file changed, 37 insertions(+), 11 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..7b764a4 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -960,11 +960,27 @@ enum drm_connector_status
>>>   analogix_dp_detect(struct drm_connector *connector, bool force)
>>>   {
>>>         struct analogix_dp_device *dp = to_dp(connector);
>>> +       enum drm_connector_status status = connector_status_disconnected;
>>> +       int ret;
>>>   -     if (analogix_dp_detect_hpd(dp))
>>> -               return connector_status_disconnected;
>>> +       if (dp->plat_data->panel && dp->dpms_mode != DRM_MODE_DPMS_ON) {
>>> +               ret = drm_panel_prepare(dp->plat_data->panel);
>>> +               if (ret) {
>>> +                       DRM_ERROR("failed to setup panel (%d)\n", ret);
>>> +                       return connector_status_disconnected;
>>> +               }
>>> +       }
>>> +
>>> +       if (!analogix_dp_detect_hpd(dp))
>>> +               status = connector_status_connected;
>>> +
>>> +       if (dp->plat_data->panel && dp->dpms_mode != DRM_MODE_DPMS_ON) {
>>> +               ret = drm_panel_unprepare(dp->plat_data->panel);
>>> +               if (ret)
>>> +                       DRM_ERROR("failed to setup the panel ret = %d\n",
>>> ret);
>>> +       }
>>>   -     return connector_status_connected;
>>> +       return status;
>>
>>
>> 1. Panel would flicker at system boot time. Your patch would flash the panel
>> power in connector->detect() function when dp->dpms_mode isn't
>> DRM_MODE_DPMS_OFF. So when userspace keep detect the connector status in
>> boot time, we could see panel would flicker (light up for a while, and turn
>> off again, and keep loop for several time). I have copied some kernel logs:
>>
>
>
> Hmm, yeah, the backlight does seem to flash. It seems like backlight
> shouldn't be on if the panel isn't enabled. this would also fix the
> issue below since you could add panel prepare/unprepare in get_modes.
>
> Alternatively, have you played around with moving things around in
> prepare/unprepare? Perhaps just enabling the supply regulator (without
> the enable gpio set) is sufficient to read HPD/EDID, but will still
> prevent the original backlight flicker/burn-in issue?
>

To answer my own question, the enable_gpio isn't populated in the dts,
so reordering things in prepare isn't feasible.

Sean


> Sean
>
>
>
>>
>> [   11.065267] YKK --------- analogix_dp_detect:1052
>> [   11.729596] YKK --------- analogix_dp_get_modes:1016
>> [   11.737608] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   11.749229] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   11.760799] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   13.315962] YKK --------- analogix_dp_detect:1052
>> [   13.984702] YKK --------- analogix_dp_get_modes:1016
>> [   13.992977] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   14.004414] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   14.015842] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   14.183109] YKK --------- analogix_dp_bridge_pre_enable:1147
>> [   14.306301] rockchip-dp ff970000.edp: Link Training Clock Recovery
>> success
>> [   14.319130] rockchip-dp ff970000.edp: Link Training success!
>> [   14.326388] rockchip-dp ff970000.edp: wait SYS_CTL_2.
>> [   14.437247] rockchip-dp ff970000.edp: Timeout of video streamclk ok
>> [   14.443585] rockchip-dp ff970000.edp: unable to config video
>>
>>
>>>   }
>>>     static void analogix_dp_connector_destroy(struct drm_connector
>>> *connector)
>>> @@ -1035,6 +1051,18 @@ static int analogix_dp_bridge_attach(struct
>>> drm_bridge *bridge)
>>>         return 0;
>>>   }
>>>   +static void analogix_dp_bridge_pre_enable(struct drm_bridge *bridge)
>>> +{
>>> +       struct analogix_dp_device *dp = bridge->driver_private;
>>> +       int ret;
>>> +
>>> +       if (dp->plat_data->panel) {
>>> +               ret = drm_panel_prepare(dp->plat_data->panel);
>>> +               if (ret)
>>> +                       DRM_ERROR("failed to setup the panel ret = %d\n",
>>> ret);
>>> +       }
>>
>>
>> 2. Driver would failed to read EDID in some case. Panel would only be
>> powered up in bridge->pre_enable() function which later than
>> connector->get_modes() function, and this would caused DPCD transfer failed
>> in analogix_dp_handle_edid(). This seem won't caused too big issue, cause
>> userspace would read EDID again after bridge/encoder is enabled. But it's
>> better to avoid this potential bug.
>>
>>
>> [   11.065267] YKK --------- analogix_dp_detect:1052
>> [   11.729596] YKK --------- analogix_dp_get_modes:1016
>> [   11.737608] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   11.749229] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   11.760799] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   13.315962] YKK --------- analogix_dp_detect:1052
>> [   13.984702] YKK --------- analogix_dp_get_modes:1016
>> [   13.992977] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   14.004414] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   14.015842] rockchip-dp ff970000.edp: analogix_dp_read_bytes_from_dpcd:
>> Aux Transaction fail!
>> [   14.183109] YKK --------- analogix_dp_bridge_pre_enable:1147
>> [   14.306301] rockchip-dp ff970000.edp: Link Training Clock Recovery
>> success
>> [   14.319130] rockchip-dp ff970000.edp: Link Training success!
>> [   14.326388] rockchip-dp ff970000.edp: wait SYS_CTL_2.
>> [   14.437247] rockchip-dp ff970000.edp: Timeout of video streamclk ok
>> [   14.443585] rockchip-dp ff970000.edp: unable to config video
>> [   14.520565] YKK --------- analogix_dp_detect:1052
>> [   14.525339] YKK --------- analogix_dp_get_modes:1016
>> [   14.531233] rockchip-dp ff970000.edp: EDID data does not include any
>> extensions.
>> [   15.595724] YKK --------- analogix_dp_detect:1052
>> [   15.600488] YKK --------- analogix_dp_get_modes:1016
>> [   15.606451] rockchip-dp ff970000.edp: EDID data does not include any
>> extensions.
>> [   17.327354] YKK --------- analogix_dp_detect:1052
>> [   17.332105] YKK --------- analogix_dp_get_modes:1016
>> [   17.338171] rockchip-dp ff970000.edp: EDID data does not include any
>> extensions.
>> [   56.236024] YKK --------- analogix_dp_detect:1052
>> [   56.240820] YKK --------- analogix_dp_get_modes:1016
>> [   56.246855] rockchip-dp ff970000.edp: EDID data does not include any
>> extensions.
>> [   64.092477] YKK --------- analogix_dp_detect:1052
>> [   64.763712] YKK --------- analogix_dp_get_modes:1016
>>
>>
>>
>> BR,
>> - Yakir
>>
>>
>>> +}
>>> +
>>>   static void analogix_dp_bridge_enable(struct drm_bridge *bridge)
>>>   {
>>>         struct analogix_dp_device *dp = bridge->driver_private;
>>> @@ -1058,6 +1086,7 @@ static void analogix_dp_bridge_enable(struct
>>> drm_bridge *bridge)
>>>   static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>>>   {
>>>         struct analogix_dp_device *dp = bridge->driver_private;
>>> +       int ret;
>>>         if (dp->dpms_mode != DRM_MODE_DPMS_ON)
>>>                 return;
>>> @@ -1077,6 +1106,10 @@ static void analogix_dp_bridge_disable(struct
>>> drm_bridge *bridge)
>>>         pm_runtime_put_sync(dp->dev);
>>>   +     ret = drm_panel_unprepare(dp->plat_data->panel);
>>> +       if (ret)
>>> +               DRM_ERROR("failed to setup the panel ret = %d\n", ret);
>>> +
>>>         dp->dpms_mode = DRM_MODE_DPMS_OFF;
>>>   }
>>>   @@ -1165,9 +1198,9 @@ static void analogix_dp_bridge_nop(struct
>>> drm_bridge *bridge)
>>>   }
>>>     static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
>>> +       .pre_enable = analogix_dp_bridge_pre_enable,
>>>         .enable = analogix_dp_bridge_enable,
>>>         .disable = analogix_dp_bridge_disable,
>>> -       .pre_enable = analogix_dp_bridge_nop,
>>>         .post_disable = analogix_dp_bridge_nop,
>>>         .mode_set = analogix_dp_bridge_mode_set,
>>>         .attach = analogix_dp_bridge_attach,
>>> @@ -1333,13 +1366,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,
>>
>>
>>
>> --
>> 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