Sean,
On 07/12/2016 11:29 PM, Sean Paul wrote:
> On Thu, Jul 7, 2016 at 7:26 PM, Yakir Yang <ykk at rock-chips.com> wrote:
>> Sean,
>>
>> Thanks for your review.
>>
>>
>> On 07/02/2016 03:46 AM, Sean Paul wrote:
>>> On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang <ykk at rock-chips.com> wrote:
>>>> The full name of PSR is Panel Self Refresh, panel device could refresh
>>>> itself with the hardware framebuffer in panel, this would make lots of
>>>> sense to save the power consumption.
>>>>
>>>> This patch have exported two symbols for platform driver to implement
>>>> the PSR function in hardware side:
>>>> - analogix_dp_active_psr()
>>>> - analogix_dp_inactive_psr()
>>>>
>>>> Signed-off-by: Yakir Yang <ykk at rock-chips.com>
>>>> ---
>>>> Changes in v3:
>>>> - split analogix_dp_enable_psr(), make it more clearly
>>>> analogix_dp_detect_sink_psr()
>>>> analogix_dp_enable_sink_psr()
>>>> - remove some nosie register setting comments
>>>>
>>>> Changes in v2:
>>>> - introduce in v2, splite the common Analogix DP changes out
>>>>
>>>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64
>>>> ++++++++++++++++++++++
>>>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 4 ++
>>>> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 54
>>>> ++++++++++++++++++
>>>> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h | 28 ++++++++++
>>>> include/drm/bridge/analogix_dp.h | 3 +
>>>> 5 files changed, 153 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>> index 32715da..b557097 100644
>>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>> @@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct
>>>> analogix_dp_device *dp)
>>>> return 0;
>>>> }
>>>>
>>>> +int analogix_dp_active_psr(struct device *dev)
>>>> +{
>>>> + struct analogix_dp_device *dp = dev_get_drvdata(dev);
>>>> +
>>>> + if (!dp->psr_support)
>>>> + return -EINVAL;
>>>> +
>>>> + analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE |
>>>> + EDP_VSC_PSR_CRC_VALUES_VALID);
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(analogix_dp_active_psr);
>>>> +
>>>> +int analogix_dp_inactive_psr(struct device *dev)
>>>> +{
>>>> + struct analogix_dp_device *dp = dev_get_drvdata(dev);
>>>> +
>>>> + if (!dp->psr_support)
>>>> + return -EINVAL;
>>>> +
>>>> + analogix_dp_send_psr_spd(dp, 0);
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr);
>>>> +
>>>> +static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
>>>> +{
>>>> + unsigned char psr_version;
>>>> +
>>>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT,
>>>> &psr_version);
>>>> + dev_info(dp->dev, "Panel PSR version : %x\n", psr_version);
>>>> +
>>> This info message is likely to be spammy since it's printed everytime
>>> the panel toggle on. Perhaps downgrade to debug level.
>>
>> Okay, done.
>>
>>>> + return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
>>>> +}
>>>> +
>>>> +static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
>>> Return type is int, but the function never fails and you don't check
>>> the return value when calling it. Seems like this should be void.
>>
>> Done.
>>
>>>> +{
>>>> + unsigned char psr_en;
>>>> +
>>>> + /* Disable psr function */
>>>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en);
>>>> + psr_en &= ~DP_PSR_ENABLE;
>>>> + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
>>>> +
>>>> + /* Main-Link transmitter remains active during PSR active states
>>>> */
>>>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en);
>>>> + psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;
>>> Why read psr_en if you're just going to overwrite it? Perhaps you meant |=
>>> here.
>>>
>> Yes, it's my mistaken, no need to read the DP_PSR_EN_CFG, just configure it
>> directly is enough.
>>
>>>> + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
>>>> +
>>>> + /* Enable psr function */
>>>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en);
>>>> + psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
>>>> + DP_PSR_CRC_VERIFICATION;
>>> Again, no need to read if you're just overwriting.
>>
>> Yes, ditto
>>
>>
>>>> + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
>>>> +
>>>> + analogix_dp_enable_psr_crc(dp);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> static unsigned char analogix_dp_calc_edid_check_sum(unsigned char
>>>> *edid_data)
>>>> {
>>>> int i;
>>>> @@ -921,6 +981,10 @@ static void analogix_dp_commit(struct
>>>> analogix_dp_device *dp)
>>>>
>>>> /* Enable video */
>>>> analogix_dp_start_video(dp);
>>>> +
>>>> + dp->psr_support = analogix_dp_detect_sink_psr(dp);
>>>> + if (dp->psr_support)
>>>> + analogix_dp_enable_sink_psr(dp);
>>>> }
>>>>
>>>> int analogix_dp_get_modes(struct drm_connector *connector)
>>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>>> index b456380..6ca5dde 100644
>>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>>> @@ -177,6 +177,7 @@ struct analogix_dp_device {
>>>> int hpd_gpio;
>>>> bool force_hpd;
>>>> unsigned char edid[EDID_BLOCK_LENGTH * 2];
>>>> + bool psr_support;
>>>>
>>>> struct analogix_dp_plat_data *plat_data;
>>>> };
>>>> @@ -278,4 +279,7 @@ int analogix_dp_is_video_stream_on(struct
>>>> analogix_dp_device *dp);
>>>> void analogix_dp_config_video_slave_mode(struct analogix_dp_device
>>>> *dp);
>>>> void analogix_dp_enable_scrambling(struct analogix_dp_device *dp);
>>>> void analogix_dp_disable_scrambling(struct analogix_dp_device *dp);
>>>> +void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp);
>>>> +void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1);
>>>> +
>>>> #endif /* _ANALOGIX_DP_CORE_H */
>>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>>> index 48030f0..e8372c7 100644
>>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>>> @@ -1322,3 +1322,57 @@ void analogix_dp_disable_scrambling(struct
>>>> analogix_dp_device *dp)
>>>> reg |= SCRAMBLING_DISABLE;
>>>> writel(reg, dp->reg_base + ANALOGIX_DP_TRAINING_PTN_SET);
>>>> }
>>>> +
>>>> +void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp)
>>>> +{
>>>> + writel(PSR_VID_CRC_FLUSH | PSR_VID_CRC_ENABLE,
>>>> + dp->reg_base + ANALOGIX_DP_CRC_CON);
>>>> +
>>>> + usleep_range(10, 20);
>>> Is this sleep arbitrary, or documented somewhere? Could you add a
>>> comment explaining how this was arrived at?
>>>
>> Yes, this sleep is arbitrary, there is no document about the
>>
> Ok, so I assume we actually need the delay for things to work? If not,
> delete the delay. If we need it, please add a comment saying the wait
> is required and the value was chosen via trial and error.
I should delete this delay, cause i found that PSR_VID_CRC_FLUSH not a
trigger bit, but a enable gate. Here is the TRM explain:
PSR_VID_CRC_FLUSH:
PSR Video CRC flush enable. The PSR video CRC value is initialized
at every v-sync leading edge.
So I just need to set this flag after enable PSR CRC function, no need
to remove it :-D
>>>> +
>>>> + writel(PSR_VID_CRC_ENABLE, dp->reg_base + ANALOGIX_DP_CRC_CON);
>>>> +}
>>>> +
>>>> +void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1)
>>>> +{
>>>> + unsigned int val;
>>>> +
>>>> + /* don't send info frame */
>>>> + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL);
>>>> + val &= ~IF_EN;
>>>> + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL);
>>>> +
>>>> + /* configure single frame update mode */
>>>> + writel(0x3, dp->reg_base + ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL);
>>>> +
>>>> + /* configure VSC HB0 ~ HB3 */
>>>> + writel(0x00, dp->reg_base + ANALOGIX_DP_SPD_HB0);
>>>> + writel(0x07, dp->reg_base + ANALOGIX_DP_SPD_HB1);
>>>> + writel(0x02, dp->reg_base + ANALOGIX_DP_SPD_HB2);
>>>> + writel(0x08, dp->reg_base + ANALOGIX_DP_SPD_HB3);
>>>> +
>>>> + /* configure VSC HB0 ~ HB3 */
>>>> + writel(0x00, dp->reg_base + ANALOGIX_DP_SPD_PB0);
>>>> + writel(0x16, dp->reg_base + ANALOGIX_DP_SPD_PB1);
>>>> + writel(0xCE, dp->reg_base + ANALOGIX_DP_SPD_PB2);
>>>> + writel(0x5D, dp->reg_base + ANALOGIX_DP_SPD_PB3);
>>>> +
>>>> + /* configure DB0 / DB1 values */
>>>> + writel(0x00, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB0);
>>> Lots of hardcoded values here. I think this could be cleaned up.
>>
>> For "HB0~HB3", "PB0~PB3" and "DB1", I don't understand very well. Those
>> seems to be a kind of head number, I got those magic values from our IC
>> side. So I think those should be okay to keep the hardcode values here :-D
>>
> Hmm. Well, I'd probably pull them out into some HBX_MAGIC_VALUE
> define, but I suppose it's fine to keep them as-is. Please at least
> add a comment above explaining that they're magic values provided by
> your IC team.
Done ;)
Thanks,
- Yakir
>> But for the "0x3" in "ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL", I would fix it
>> with suitable macro.
>>
>>
>>>> + writel(db1, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB1);
>>>> +
>>>> + /* set reuse spd inforframe */
>>>> + val = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3);
>>>> + val |= REUSE_SPD_EN;
>>>> + writel(val, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3);
>>>> +
>>>> + /* mark info frame update */
>>>> + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL);
>>>> + val = (val | IF_UP) & ~IF_EN;
>>>> + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL);
>>>> +
>>>> + /* send info frame */
>>>> + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL);
>>>> + val |= IF_EN;
>>>> + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL);
>>>> +}
>>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
>>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
>>>> index cdcc6c5..a2698e4 100644
>>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
>>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
>>>> @@ -22,6 +22,8 @@
>>>> #define ANALOGIX_DP_VIDEO_CTL_8 0x3C
>>>> #define ANALOGIX_DP_VIDEO_CTL_10 0x44
>>>>
>>>> +#define ANALOGIX_DP_SPDIF_AUDIO_CTL_0 0xD8
>>>> +
>>>> #define ANALOGIX_DP_PLL_REG_1 0xfc
>>>> #define ANALOGIX_DP_PLL_REG_2 0x9e4
>>>> #define ANALOGIX_DP_PLL_REG_3 0x9e8
>>>> @@ -30,6 +32,21 @@
>>>>
>>>> #define ANALOGIX_DP_PD 0x12c
>>>>
>>>> +#define ANALOGIX_DP_IF_TYPE 0x244
>>>> +#define ANALOGIX_DP_IF_PKT_DB1 0x254
>>>> +#define ANALOGIX_DP_IF_PKT_DB2 0x258
>>>> +#define ANALOGIX_DP_SPD_HB0 0x2F8
>>>> +#define ANALOGIX_DP_SPD_HB1 0x2FC
>>>> +#define ANALOGIX_DP_SPD_HB2 0x300
>>>> +#define ANALOGIX_DP_SPD_HB3 0x304
>>>> +#define ANALOGIX_DP_SPD_PB0 0x308
>>>> +#define ANALOGIX_DP_SPD_PB1 0x30C
>>>> +#define ANALOGIX_DP_SPD_PB2 0x310
>>>> +#define ANALOGIX_DP_SPD_PB3 0x314
>>>> +#define ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL 0x318
>>>> +#define ANALOGIX_DP_VSC_SHADOW_DB0 0x31C
>>>> +#define ANALOGIX_DP_VSC_SHADOW_DB1 0x320
>>>> +
>>>> #define ANALOGIX_DP_LANE_MAP 0x35C
>>>>
>>>> #define ANALOGIX_DP_ANALOG_CTL_1 0x370
>>>> @@ -103,6 +120,8 @@
>>>>
>>>> #define ANALOGIX_DP_SOC_GENERAL_CTL 0x800
>>>>
>>>> +#define ANALOGIX_DP_CRC_CON 0x890
>>>> +
>>>> /* ANALOGIX_DP_TX_SW_RESET */
>>>> #define RESET_DP_TX (0x1 << 0)
>>>>
>>>> @@ -151,6 +170,7 @@
>>>> #define VID_CHK_UPDATE_TYPE_SHIFT (4)
>>>> #define VID_CHK_UPDATE_TYPE_1 (0x1 << 4)
>>>> #define VID_CHK_UPDATE_TYPE_0 (0x0 << 4)
>>>> +#define REUSE_SPD_EN (0x1 << 3)
>>>>
>>>> /* ANALOGIX_DP_VIDEO_CTL_8 */
>>>> #define VID_HRES_TH(x) (((x) & 0xf) << 4)
>>>> @@ -376,4 +396,12 @@
>>>> #define VIDEO_MODE_SLAVE_MODE (0x1 << 0)
>>>> #define VIDEO_MODE_MASTER_MODE (0x0 << 0)
>>>>
>>>> +/* ANALOGIX_DP_PKT_SEND_CTL */
>>>> +#define IF_UP (0x1 << 4)
>>>> +#define IF_EN (0x1 << 0)
>>>> +
>>>> +/* ANALOGIX_DP_CRC_CON */
>>>> +#define PSR_VID_CRC_FLUSH (0x1 << 2)
>>>> +#define PSR_VID_CRC_ENABLE (0x1 << 0)
>>>> +
>>>> #endif /* _ANALOGIX_DP_REG_H */
>>>> diff --git a/include/drm/bridge/analogix_dp.h
>>>> b/include/drm/bridge/analogix_dp.h
>>>> index 261b86d..183a336 100644
>>>> --- a/include/drm/bridge/analogix_dp.h
>>>> +++ b/include/drm/bridge/analogix_dp.h
>>>> @@ -38,6 +38,9 @@ struct analogix_dp_plat_data {
>>>> struct drm_connector *);
>>>> };
>>>>
>>>> +int analogix_dp_active_psr(struct device *dev);
>>>> +int analogix_dp_inactive_psr(struct device *dev);
>>> Why active/inactive instead of enable/disable, which is used everywhere
>>> else?
>>
>> Done
>>
>> Thanks,
>> - Yakir
>>
>>
>>>> +
>>>> int analogix_dp_resume(struct device *dev);
>>>> int analogix_dp_suspend(struct device *dev);
>>>>
>>>> --
>>>> 1.9.1
>>>>
>>>>
>>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://lists.freedesktop.org/archives/dri-devel/attachments/20160713/e045f779/attachment-0001.html>
