* [PATCH] drm/bridge: analogix: Don't return -EINVAL when panel not support PSR in PSR functions
@ 2016-12-02 2:32 zain wang
2016-12-02 3:54 ` Archit Taneja
0 siblings, 1 reply; 5+ messages in thread
From: zain wang @ 2016-12-02 2:32 UTC (permalink / raw)
To: Sean Paul, Daniel Vetter, Inki Dae, David Airlie
Cc: Tomeu Vizoso, Mika Kahola, Stéphane Marchesin, Tomasz Figa,
dianders, Thierry Reding, Krzysztof Kozlowski, Heiko Stuebner,
Jingoo Han, Javier Martinez Canillas, linux-kernel, dri-devel,
linux-samsung-soc, linux-rockchip, zain wang
We will ignored PSR setting if panel not support it. So, in this case, we should
return from analogix_dp_enable/disable_psr() without any error code.
Let's retrun 0 instead of -EINVAL when panel not support PSR in
analogix_dp_enable/disable_psr().
Signed-off-by: zain wang <wzz@rock-chips.com>
---
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 6e0447f..0cb3695 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -112,7 +112,7 @@ int analogix_dp_enable_psr(struct device *dev)
struct edp_vsc_psr psr_vsc;
if (!dp->psr_support)
- return -EINVAL;
+ return 0;
/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
memset(&psr_vsc, 0, sizeof(psr_vsc));
@@ -135,7 +135,7 @@ int analogix_dp_disable_psr(struct device *dev)
struct edp_vsc_psr psr_vsc;
if (!dp->psr_support)
- return -EINVAL;
+ return 0;
/* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
memset(&psr_vsc, 0, sizeof(psr_vsc));
@@ -878,6 +878,8 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
dp->psr_support = analogix_dp_detect_sink_psr(dp);
if (dp->psr_support)
analogix_dp_enable_sink_psr(dp);
+ else
+ dev_warn(dp->dev, "Sink not support PSR\n");
}
/*
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/bridge: analogix: Don't return -EINVAL when panel not support PSR in PSR functions
2016-12-02 2:32 [PATCH] drm/bridge: analogix: Don't return -EINVAL when panel not support PSR in PSR functions zain wang
@ 2016-12-02 3:54 ` Archit Taneja
2016-12-02 16:03 ` Sean Paul
0 siblings, 1 reply; 5+ messages in thread
From: Archit Taneja @ 2016-12-02 3:54 UTC (permalink / raw)
To: zain wang, Sean Paul, Daniel Vetter, Inki Dae, David Airlie
Cc: Tomeu Vizoso, Mika Kahola, Stéphane Marchesin, Tomasz Figa,
dianders, Thierry Reding, Krzysztof Kozlowski, Heiko Stuebner,
Jingoo Han, Javier Martinez Canillas, linux-kernel, dri-devel,
linux-samsung-soc, linux-rockchip
Hi,
On 12/02/2016 08:02 AM, zain wang wrote:
> We will ignored PSR setting if panel not support it. So, in this case, we should
> return from analogix_dp_enable/disable_psr() without any error code.
> Let's retrun 0 instead of -EINVAL when panel not support PSR in
> analogix_dp_enable/disable_psr().
>
> Signed-off-by: zain wang <wzz@rock-chips.com>
> ---
> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 6e0447f..0cb3695 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -112,7 +112,7 @@ int analogix_dp_enable_psr(struct device *dev)
> struct edp_vsc_psr psr_vsc;
>
> if (!dp->psr_support)
> - return -EINVAL;
> + return 0;
Looking at the rockchip analogix dp code, in analogix_dp_psr_set, the worker that calls
analogix_dp_enable/disable_psr isn't even if psr isn't enabled. So, the bridge funcs
shouldn't be called in the first place. I think the error handling is fine to have
here.
>
> /* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> memset(&psr_vsc, 0, sizeof(psr_vsc));
> @@ -135,7 +135,7 @@ int analogix_dp_disable_psr(struct device *dev)
> struct edp_vsc_psr psr_vsc;
>
> if (!dp->psr_support)
> - return -EINVAL;
> + return 0;
>
> /* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
> memset(&psr_vsc, 0, sizeof(psr_vsc));
> @@ -878,6 +878,8 @@ static void analogix_dp_commit(struct analogix_dp_device *dp)
> dp->psr_support = analogix_dp_detect_sink_psr(dp);
> if (dp->psr_support)
> analogix_dp_enable_sink_psr(dp);
> + else
> + dev_warn(dp->dev, "Sink not support PSR\n");
This doesn't seem beneficial either. There seems to be a debug
print already in analogix_dp_detect_sink_psr which reports PSR
related info.
Archit
> }
>
> /*
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/bridge: analogix: Don't return -EINVAL when panel not support PSR in PSR functions
2016-12-02 3:54 ` Archit Taneja
@ 2016-12-02 16:03 ` Sean Paul
2016-12-05 3:13 ` Archit Taneja
0 siblings, 1 reply; 5+ messages in thread
From: Sean Paul @ 2016-12-02 16:03 UTC (permalink / raw)
To: Archit Taneja
Cc: zain wang, Daniel Vetter, Inki Dae, David Airlie, Tomeu Vizoso,
Mika Kahola, Stéphane Marchesin, Tomasz Figa, Doug Anderson,
Thierry Reding, Krzysztof Kozlowski, Heiko Stuebner, Jingoo Han,
Javier Martinez Canillas, Linux Kernel Mailing List, dri-devel,
linux-samsung-soc, linux-rockchip
On Thu, Dec 1, 2016 at 10:54 PM, Archit Taneja <architt@codeaurora.org> wrote:
> Hi,
>
> On 12/02/2016 08:02 AM, zain wang wrote:
>>
>> We will ignored PSR setting if panel not support it. So, in this case, we
>> should
>> return from analogix_dp_enable/disable_psr() without any error code.
>> Let's retrun 0 instead of -EINVAL when panel not support PSR in
>> analogix_dp_enable/disable_psr().
>>
>> Signed-off-by: zain wang <wzz@rock-chips.com>
>> ---
>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> index 6e0447f..0cb3695 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -112,7 +112,7 @@ int analogix_dp_enable_psr(struct device *dev)
>> struct edp_vsc_psr psr_vsc;
>>
>> if (!dp->psr_support)
>> - return -EINVAL;
>> + return 0;
>
>
> Looking at the rockchip analogix dp code, in analogix_dp_psr_set, the worker
> that calls
> analogix_dp_enable/disable_psr isn't even if psr isn't enabled. So, the
> bridge funcs
> shouldn't be called in the first place. I think the error handling is fine
> to have
> here.
>
Hi Archit,
This was my first impression, too, and the complexity of the various
psr abstraction layers don't help :)
However, this code path will be hit if the source supports psr, but
the sink doesn't. The rockchip_drm_psr code doesn't know if the sink
supports psr, so it will end up calling this.
Sean
>>
>> /* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
>> memset(&psr_vsc, 0, sizeof(psr_vsc));
>> @@ -135,7 +135,7 @@ int analogix_dp_disable_psr(struct device *dev)
>> struct edp_vsc_psr psr_vsc;
>>
>> if (!dp->psr_support)
>> - return -EINVAL;
>> + return 0;
>>
>> /* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
>> memset(&psr_vsc, 0, sizeof(psr_vsc));
>> @@ -878,6 +878,8 @@ static void analogix_dp_commit(struct
>> analogix_dp_device *dp)
>> dp->psr_support = analogix_dp_detect_sink_psr(dp);
>> if (dp->psr_support)
>> analogix_dp_enable_sink_psr(dp);
>> + else
>> + dev_warn(dp->dev, "Sink not support PSR\n");
>
>
> This doesn't seem beneficial either. There seems to be a debug
> print already in analogix_dp_detect_sink_psr which reports PSR
> related info.
>
> Archit
>
>> }
>>
>> /*
>>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/bridge: analogix: Don't return -EINVAL when panel not support PSR in PSR functions
2016-12-02 16:03 ` Sean Paul
@ 2016-12-05 3:13 ` Archit Taneja
2016-12-05 17:28 ` Sean Paul
0 siblings, 1 reply; 5+ messages in thread
From: Archit Taneja @ 2016-12-05 3:13 UTC (permalink / raw)
To: Sean Paul
Cc: zain wang, Daniel Vetter, Inki Dae, David Airlie, Tomeu Vizoso,
Mika Kahola, Stéphane Marchesin, Tomasz Figa, Doug Anderson,
Thierry Reding, Krzysztof Kozlowski, Heiko Stuebner, Jingoo Han,
Javier Martinez Canillas, Linux Kernel Mailing List, dri-devel,
linux-samsung-soc, linux-rockchip
On 12/02/2016 09:33 PM, Sean Paul wrote:
> On Thu, Dec 1, 2016 at 10:54 PM, Archit Taneja <architt@codeaurora.org> wrote:
>> Hi,
>>
>> On 12/02/2016 08:02 AM, zain wang wrote:
>>>
>>> We will ignored PSR setting if panel not support it. So, in this case, we
>>> should
>>> return from analogix_dp_enable/disable_psr() without any error code.
>>> Let's retrun 0 instead of -EINVAL when panel not support PSR in
>>> analogix_dp_enable/disable_psr().
>>>
>>> Signed-off-by: zain wang <wzz@rock-chips.com>
>>> ---
>>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 6 ++++--
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> index 6e0447f..0cb3695 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -112,7 +112,7 @@ int analogix_dp_enable_psr(struct device *dev)
>>> struct edp_vsc_psr psr_vsc;
>>>
>>> if (!dp->psr_support)
>>> - return -EINVAL;
>>> + return 0;
>>
>>
>> Looking at the rockchip analogix dp code, in analogix_dp_psr_set, the worker
>> that calls
>> analogix_dp_enable/disable_psr isn't even if psr isn't enabled. So, the
>> bridge funcs
>> shouldn't be called in the first place. I think the error handling is fine
>> to have
>> here.
>>
>
> Hi Archit,
>
> This was my first impression, too, and the complexity of the various
> psr abstraction layers don't help :)
>
> However, this code path will be hit if the source supports psr, but
> the sink doesn't. The rockchip_drm_psr code doesn't know if the sink
> supports psr, so it will end up calling this.
Okay, thanks for the explanation. The dev_warn() below still seems
unnecessary, right?
Archit
>
> Sean
>
>
>>>
>>> /* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
>>> memset(&psr_vsc, 0, sizeof(psr_vsc));
>>> @@ -135,7 +135,7 @@ int analogix_dp_disable_psr(struct device *dev)
>>> struct edp_vsc_psr psr_vsc;
>>>
>>> if (!dp->psr_support)
>>> - return -EINVAL;
>>> + return 0;
>>>
>>> /* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
>>> memset(&psr_vsc, 0, sizeof(psr_vsc));
>>> @@ -878,6 +878,8 @@ static void analogix_dp_commit(struct
>>> analogix_dp_device *dp)
>>> dp->psr_support = analogix_dp_detect_sink_psr(dp);
>>> if (dp->psr_support)
>>> analogix_dp_enable_sink_psr(dp);
>>> + else
>>> + dev_warn(dp->dev, "Sink not support PSR\n");
>>
>>
>> This doesn't seem beneficial either. There seems to be a debug
>> print already in analogix_dp_detect_sink_psr which reports PSR
>> related info.
>>
>> Archit
>>
>>> }
>>>
>>> /*
>>>
>>
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/bridge: analogix: Don't return -EINVAL when panel not support PSR in PSR functions
2016-12-05 3:13 ` Archit Taneja
@ 2016-12-05 17:28 ` Sean Paul
0 siblings, 0 replies; 5+ messages in thread
From: Sean Paul @ 2016-12-05 17:28 UTC (permalink / raw)
To: Archit Taneja
Cc: zain wang, Daniel Vetter, Inki Dae, David Airlie, Tomeu Vizoso,
Mika Kahola, Stéphane Marchesin, Tomasz Figa, Doug Anderson,
Thierry Reding, Krzysztof Kozlowski, Heiko Stuebner, Jingoo Han,
Javier Martinez Canillas, Linux Kernel Mailing List, dri-devel,
linux-samsung-soc, linux-rockchip
On Sun, Dec 4, 2016 at 10:13 PM, Archit Taneja <architt@codeaurora.org> wrote:
>
>
> On 12/02/2016 09:33 PM, Sean Paul wrote:
>>
>> On Thu, Dec 1, 2016 at 10:54 PM, Archit Taneja <architt@codeaurora.org>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 12/02/2016 08:02 AM, zain wang wrote:
>>>>
>>>>
>>>> We will ignored PSR setting if panel not support it. So, in this case,
>>>> we
>>>> should
>>>> return from analogix_dp_enable/disable_psr() without any error code.
>>>> Let's retrun 0 instead of -EINVAL when panel not support PSR in
>>>> analogix_dp_enable/disable_psr().
>>>>
>>>> Signed-off-by: zain wang <wzz@rock-chips.com>
>>>> ---
>>>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 6 ++++--
>>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>> index 6e0447f..0cb3695 100644
>>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>>> @@ -112,7 +112,7 @@ int analogix_dp_enable_psr(struct device *dev)
>>>> struct edp_vsc_psr psr_vsc;
>>>>
>>>> if (!dp->psr_support)
>>>> - return -EINVAL;
>>>> + return 0;
>>>
>>>
>>>
>>> Looking at the rockchip analogix dp code, in analogix_dp_psr_set, the
>>> worker
>>> that calls
>>> analogix_dp_enable/disable_psr isn't even if psr isn't enabled. So, the
>>> bridge funcs
>>> shouldn't be called in the first place. I think the error handling is
>>> fine
>>> to have
>>> here.
>>>
>>
>> Hi Archit,
>>
>> This was my first impression, too, and the complexity of the various
>> psr abstraction layers don't help :)
>>
>> However, this code path will be hit if the source supports psr, but
>> the sink doesn't. The rockchip_drm_psr code doesn't know if the sink
>> supports psr, so it will end up calling this.
>
>
> Okay, thanks for the explanation. The dev_warn() below still seems
> unnecessary, right?
>
Yeah, one could make a case for dev_info (disclaimer: I have a high
tolerance for noisy logs), but a warning does seem excessive.
Sean
> Archit
>
>
>>
>> Sean
>>
>>
>>>>
>>>> /* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
>>>> memset(&psr_vsc, 0, sizeof(psr_vsc));
>>>> @@ -135,7 +135,7 @@ int analogix_dp_disable_psr(struct device *dev)
>>>> struct edp_vsc_psr psr_vsc;
>>>>
>>>> if (!dp->psr_support)
>>>> - return -EINVAL;
>>>> + return 0;
>>>>
>>>> /* Prepare VSC packet as per EDP 1.4 spec, Table 6.9 */
>>>> memset(&psr_vsc, 0, sizeof(psr_vsc));
>>>> @@ -878,6 +878,8 @@ static void analogix_dp_commit(struct
>>>> analogix_dp_device *dp)
>>>> dp->psr_support = analogix_dp_detect_sink_psr(dp);
>>>> if (dp->psr_support)
>>>> analogix_dp_enable_sink_psr(dp);
>>>> + else
>>>> + dev_warn(dp->dev, "Sink not support PSR\n");
>>>
>>>
>>>
>>> This doesn't seem beneficial either. There seems to be a debug
>>> print already in analogix_dp_detect_sink_psr which reports PSR
>>> related info.
>>>
>>> Archit
>>>
>>>> }
>>>>
>>>> /*
>>>>
>>>
>>> --
>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>>> a Linux Foundation Collaborative Project
>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-12-05 17:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-02 2:32 [PATCH] drm/bridge: analogix: Don't return -EINVAL when panel not support PSR in PSR functions zain wang
2016-12-02 3:54 ` Archit Taneja
2016-12-02 16:03 ` Sean Paul
2016-12-05 3:13 ` Archit Taneja
2016-12-05 17:28 ` Sean Paul
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).