MSA MISC0 bit 1 to 7 contains Colorimetry Indicator Field. At
current implementation, at DP_TEST_DYNAMIC_RANGE_CEA case the
Colorimetry Indicator Field is mistakenly left shifted one extra
bit. This patch return correct value of colorimetry at
dp_link_get_colorimetry_config() to fix this problem.
Changes in V2:
-- drop retrieving colorimetry from colorspace
-- drop dr = link->dp_link.test_video.test_dyn_range assignment
Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
Signed-off-by: Kuogee Hsieh <[email protected]>
---
drivers/gpu/drm/msm/dp/dp_link.c | 11 ++++++-----
drivers/gpu/drm/msm/dp/dp_link.h | 3 +++
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
index 98427d4..2e1bdaf 100644
--- a/drivers/gpu/drm/msm/dp/dp_link.c
+++ b/drivers/gpu/drm/msm/dp/dp_link.c
@@ -1082,7 +1082,7 @@ int dp_link_process_request(struct dp_link *dp_link)
int dp_link_get_colorimetry_config(struct dp_link *dp_link)
{
- u32 cc;
+ u32 cc = DP_MISC0_LEGACY_RGB;
struct dp_link_private *link;
if (!dp_link) {
@@ -1096,10 +1096,11 @@ int dp_link_get_colorimetry_config(struct dp_link *dp_link)
* Unless a video pattern CTS test is ongoing, use RGB_VESA
* Only RGB_VESA and RGB_CEA supported for now
*/
- if (dp_link_is_video_pattern_requested(link))
- cc = link->dp_link.test_video.test_dyn_range;
- else
- cc = DP_TEST_DYNAMIC_RANGE_VESA;
+ if (dp_link_is_video_pattern_requested(link)) {
+ if (link->dp_link.test_video.test_dyn_range &
+ DP_TEST_DYNAMIC_RANGE_CEA)
+ cc = DP_MISC0_CEA_RGB;
+ }
return cc;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_link.h b/drivers/gpu/drm/msm/dp/dp_link.h
index 9dd4dd9..fe8f716 100644
--- a/drivers/gpu/drm/msm/dp/dp_link.h
+++ b/drivers/gpu/drm/msm/dp/dp_link.h
@@ -12,6 +12,9 @@
#define DP_TEST_BIT_DEPTH_UNKNOWN 0xFFFFFFFF
#define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
+#define DP_MISC0_LEGACY_RGB 0
+#define DP_MISC0_CEA_RGB 0x04
+
struct dp_link_info {
unsigned char revision;
unsigned int rate;
--
2.7.4
On Wed, 17 Jan 2024 at 19:54, Kuogee Hsieh <[email protected]> wrote:
>
> MSA MISC0 bit 1 to 7 contains Colorimetry Indicator Field. At
> current implementation, at DP_TEST_DYNAMIC_RANGE_CEA case the
In the current implementation, in the ... case
> Colorimetry Indicator Field is mistakenly left shifted one extra
> bit.
This doesn't make sense. You say that the value is mistakenly shifted,
but the shift is still in place in dp_catalog_ctrl_config_misc().
> This patch return correct value of colorimetry at
> dp_link_get_colorimetry_config() to fix this problem.
See Documentation/process/submitting-patches.rst#_describe_changes
>
> Changes in V2:
> -- drop retrieving colorimetry from colorspace
> -- drop dr = link->dp_link.test_video.test_dyn_range assignment
>
> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> Signed-off-by: Kuogee Hsieh <[email protected]>
> ---
> drivers/gpu/drm/msm/dp/dp_link.c | 11 ++++++-----
> drivers/gpu/drm/msm/dp/dp_link.h | 3 +++
> 2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
> index 98427d4..2e1bdaf 100644
> --- a/drivers/gpu/drm/msm/dp/dp_link.c
> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> @@ -1082,7 +1082,7 @@ int dp_link_process_request(struct dp_link *dp_link)
>
> int dp_link_get_colorimetry_config(struct dp_link *dp_link)
> {
> - u32 cc;
> + u32 cc = DP_MISC0_LEGACY_RGB;
> struct dp_link_private *link;
>
> if (!dp_link) {
> @@ -1096,10 +1096,11 @@ int dp_link_get_colorimetry_config(struct dp_link *dp_link)
> * Unless a video pattern CTS test is ongoing, use RGB_VESA
> * Only RGB_VESA and RGB_CEA supported for now
> */
> - if (dp_link_is_video_pattern_requested(link))
> - cc = link->dp_link.test_video.test_dyn_range;
> - else
> - cc = DP_TEST_DYNAMIC_RANGE_VESA;
> + if (dp_link_is_video_pattern_requested(link)) {
> + if (link->dp_link.test_video.test_dyn_range &
> + DP_TEST_DYNAMIC_RANGE_CEA)
> + cc = DP_MISC0_CEA_RGB;
> + }
>
> return cc;
> }
> diff --git a/drivers/gpu/drm/msm/dp/dp_link.h b/drivers/gpu/drm/msm/dp/dp_link.h
> index 9dd4dd9..fe8f716 100644
> --- a/drivers/gpu/drm/msm/dp/dp_link.h
> +++ b/drivers/gpu/drm/msm/dp/dp_link.h
> @@ -12,6 +12,9 @@
> #define DP_TEST_BIT_DEPTH_UNKNOWN 0xFFFFFFFF
> #define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
>
> +#define DP_MISC0_LEGACY_RGB 0
> +#define DP_MISC0_CEA_RGB 0x04
These should go to dp_reg.h and should start with DP_MISC0_COLORIMETRY_CFG
> +
> struct dp_link_info {
> unsigned char revision;
> unsigned int rate;
> --
> 2.7.4
>
--
With best wishes
Dmitry
On 1/17/2024 10:12 AM, Dmitry Baryshkov wrote:
> On Wed, 17 Jan 2024 at 19:54, Kuogee Hsieh <[email protected]> wrote:
>> MSA MISC0 bit 1 to 7 contains Colorimetry Indicator Field. At
>> current implementation, at DP_TEST_DYNAMIC_RANGE_CEA case the
> In the current implementation, in the ... case
>
>> Colorimetry Indicator Field is mistakenly left shifted one extra
>> bit.
> This doesn't make sense. You say that the value is mistakenly shifted,
> but the shift is still in place in dp_catalog_ctrl_config_misc().
The problem is at
link->dp_link.test_video.test_dyn_range = (bp &
DP_TEST_DYNAMIC_RANGE_CEA); <== this from reading dpcd directly where
==> DP_TEST_DYNAMIC_RANGE_CEA is (1 << 3)
within dp_catalog_ctrl_config_misc(), cc will be left shift one more bit.
so that cc is totally is left shifted 4 bits (bit 4).
at misc0, it should be bit 3 set only for CEA_RGB.
>
>> This patch return correct value of colorimetry at
>> dp_link_get_colorimetry_config() to fix this problem.
> See Documentation/process/submitting-patches.rst#_describe_changes
>
>> Changes in V2:
>> -- drop retrieving colorimetry from colorspace
>> -- drop dr = link->dp_link.test_video.test_dyn_range assignment
>>
>> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
>> Signed-off-by: Kuogee Hsieh <[email protected]>
>> ---
>> drivers/gpu/drm/msm/dp/dp_link.c | 11 ++++++-----
>> drivers/gpu/drm/msm/dp/dp_link.h | 3 +++
>> 2 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
>> index 98427d4..2e1bdaf 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_link.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
>> @@ -1082,7 +1082,7 @@ int dp_link_process_request(struct dp_link *dp_link)
>>
>> int dp_link_get_colorimetry_config(struct dp_link *dp_link)
>> {
>> - u32 cc;
>> + u32 cc = DP_MISC0_LEGACY_RGB;
>> struct dp_link_private *link;
>>
>> if (!dp_link) {
>> @@ -1096,10 +1096,11 @@ int dp_link_get_colorimetry_config(struct dp_link *dp_link)
>> * Unless a video pattern CTS test is ongoing, use RGB_VESA
>> * Only RGB_VESA and RGB_CEA supported for now
>> */
>> - if (dp_link_is_video_pattern_requested(link))
>> - cc = link->dp_link.test_video.test_dyn_range;
>> - else
>> - cc = DP_TEST_DYNAMIC_RANGE_VESA;
>> + if (dp_link_is_video_pattern_requested(link)) {
>> + if (link->dp_link.test_video.test_dyn_range &
>> + DP_TEST_DYNAMIC_RANGE_CEA)
>> + cc = DP_MISC0_CEA_RGB;
>> + }
>>
>> return cc;
>> }
>> diff --git a/drivers/gpu/drm/msm/dp/dp_link.h b/drivers/gpu/drm/msm/dp/dp_link.h
>> index 9dd4dd9..fe8f716 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_link.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_link.h
>> @@ -12,6 +12,9 @@
>> #define DP_TEST_BIT_DEPTH_UNKNOWN 0xFFFFFFFF
>> #define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
>>
>> +#define DP_MISC0_LEGACY_RGB 0
>> +#define DP_MISC0_CEA_RGB 0x04
> These should go to dp_reg.h and should start with DP_MISC0_COLORIMETRY_CFG
>
>> +
>> struct dp_link_info {
>> unsigned char revision;
>> unsigned int rate;
>> --
>> 2.7.4
>>
>
On Wed, 17 Jan 2024 at 20:29, Kuogee Hsieh <[email protected]> wrote:
>
>
> On 1/17/2024 10:12 AM, Dmitry Baryshkov wrote:
> > On Wed, 17 Jan 2024 at 19:54, Kuogee Hsieh <[email protected]> wrote:
> >> MSA MISC0 bit 1 to 7 contains Colorimetry Indicator Field. At
> >> current implementation, at DP_TEST_DYNAMIC_RANGE_CEA case the
> > In the current implementation, in the ... case
> >
> >> Colorimetry Indicator Field is mistakenly left shifted one extra
> >> bit.
> > This doesn't make sense. You say that the value is mistakenly shifted,
> > but the shift is still in place in dp_catalog_ctrl_config_misc().
>
> The problem is at
>
> link->dp_link.test_video.test_dyn_range = (bp &
> DP_TEST_DYNAMIC_RANGE_CEA); <== this from reading dpcd directly where
> ==> DP_TEST_DYNAMIC_RANGE_CEA is (1 << 3)
>
> within dp_catalog_ctrl_config_misc(), cc will be left shift one more bit.
> so that cc is totally is left shifted 4 bits (bit 4).
>
> at misc0, it should be bit 3 set only for CEA_RGB.
Yes. But your patch doesn't fix the shift (which is correct). You
patch fixes the value being written to that field.
>
> >
> >> This patch return correct value of colorimetry at
> >> dp_link_get_colorimetry_config() to fix this problem.
> > See Documentation/process/submitting-patches.rst#_describe_changes
> >
> >> Changes in V2:
> >> -- drop retrieving colorimetry from colorspace
> >> -- drop dr = link->dp_link.test_video.test_dyn_range assignment
> >>
> >> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
> >> Signed-off-by: Kuogee Hsieh <[email protected]>
> >> ---
> >> drivers/gpu/drm/msm/dp/dp_link.c | 11 ++++++-----
> >> drivers/gpu/drm/msm/dp/dp_link.h | 3 +++
> >> 2 files changed, 9 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
> >> index 98427d4..2e1bdaf 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_link.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> >> @@ -1082,7 +1082,7 @@ int dp_link_process_request(struct dp_link *dp_link)
> >>
> >> int dp_link_get_colorimetry_config(struct dp_link *dp_link)
> >> {
> >> - u32 cc;
> >> + u32 cc = DP_MISC0_LEGACY_RGB;
> >> struct dp_link_private *link;
> >>
> >> if (!dp_link) {
> >> @@ -1096,10 +1096,11 @@ int dp_link_get_colorimetry_config(struct dp_link *dp_link)
> >> * Unless a video pattern CTS test is ongoing, use RGB_VESA
> >> * Only RGB_VESA and RGB_CEA supported for now
> >> */
> >> - if (dp_link_is_video_pattern_requested(link))
> >> - cc = link->dp_link.test_video.test_dyn_range;
> >> - else
> >> - cc = DP_TEST_DYNAMIC_RANGE_VESA;
> >> + if (dp_link_is_video_pattern_requested(link)) {
> >> + if (link->dp_link.test_video.test_dyn_range &
> >> + DP_TEST_DYNAMIC_RANGE_CEA)
> >> + cc = DP_MISC0_CEA_RGB;
> >> + }
> >>
> >> return cc;
> >> }
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_link.h b/drivers/gpu/drm/msm/dp/dp_link.h
> >> index 9dd4dd9..fe8f716 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_link.h
> >> +++ b/drivers/gpu/drm/msm/dp/dp_link.h
> >> @@ -12,6 +12,9 @@
> >> #define DP_TEST_BIT_DEPTH_UNKNOWN 0xFFFFFFFF
> >> #define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
> >>
> >> +#define DP_MISC0_LEGACY_RGB 0
> >> +#define DP_MISC0_CEA_RGB 0x04
> > These should go to dp_reg.h and should start with DP_MISC0_COLORIMETRY_CFG
> >
> >> +
> >> struct dp_link_info {
> >> unsigned char revision;
> >> unsigned int rate;
> >> --
> >> 2.7.4
> >>
> >
--
With best wishes
Dmitry
On 1/17/2024 10:40 AM, Dmitry Baryshkov wrote:
> On Wed, 17 Jan 2024 at 20:29, Kuogee Hsieh <[email protected]> wrote:
>>
>> On 1/17/2024 10:12 AM, Dmitry Baryshkov wrote:
>>> On Wed, 17 Jan 2024 at 19:54, Kuogee Hsieh <[email protected]> wrote:
>>>> MSA MISC0 bit 1 to 7 contains Colorimetry Indicator Field. At
>>>> current implementation, at DP_TEST_DYNAMIC_RANGE_CEA case the
>>> In the current implementation, in the ... case
>>>
>>>> Colorimetry Indicator Field is mistakenly left shifted one extra
>>>> bit.
>>> This doesn't make sense. You say that the value is mistakenly shifted,
>>> but the shift is still in place in dp_catalog_ctrl_config_misc().
>> The problem is at
>>
>> link->dp_link.test_video.test_dyn_range = (bp &
>> DP_TEST_DYNAMIC_RANGE_CEA); <== this from reading dpcd directly where
>> ==> DP_TEST_DYNAMIC_RANGE_CEA is (1 << 3)
>>
>> within dp_catalog_ctrl_config_misc(), cc will be left shift one more bit.
>> so that cc is totally is left shifted 4 bits (bit 4).
>>
>> at misc0, it should be bit 3 set only for CEA_RGB.
> Yes. But your patch doesn't fix the shift (which is correct). You
> patch fixes the value being written to that field.
ok, i will rewording the commit test
>
>>>> This patch return correct value of colorimetry at
>>>> dp_link_get_colorimetry_config() to fix this problem.
>>> See Documentation/process/submitting-patches.rst#_describe_changes
>>>
>>>> Changes in V2:
>>>> -- drop retrieving colorimetry from colorspace
>>>> -- drop dr = link->dp_link.test_video.test_dyn_range assignment
>>>>
>>>> Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support")
>>>> Signed-off-by: Kuogee Hsieh <[email protected]>
>>>> ---
>>>> drivers/gpu/drm/msm/dp/dp_link.c | 11 ++++++-----
>>>> drivers/gpu/drm/msm/dp/dp_link.h | 3 +++
>>>> 2 files changed, 9 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
>>>> index 98427d4..2e1bdaf 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_link.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
>>>> @@ -1082,7 +1082,7 @@ int dp_link_process_request(struct dp_link *dp_link)
>>>>
>>>> int dp_link_get_colorimetry_config(struct dp_link *dp_link)
>>>> {
>>>> - u32 cc;
>>>> + u32 cc = DP_MISC0_LEGACY_RGB;
>>>> struct dp_link_private *link;
>>>>
>>>> if (!dp_link) {
>>>> @@ -1096,10 +1096,11 @@ int dp_link_get_colorimetry_config(struct dp_link *dp_link)
>>>> * Unless a video pattern CTS test is ongoing, use RGB_VESA
>>>> * Only RGB_VESA and RGB_CEA supported for now
>>>> */
>>>> - if (dp_link_is_video_pattern_requested(link))
>>>> - cc = link->dp_link.test_video.test_dyn_range;
>>>> - else
>>>> - cc = DP_TEST_DYNAMIC_RANGE_VESA;
>>>> + if (dp_link_is_video_pattern_requested(link)) {
>>>> + if (link->dp_link.test_video.test_dyn_range &
>>>> + DP_TEST_DYNAMIC_RANGE_CEA)
>>>> + cc = DP_MISC0_CEA_RGB;
>>>> + }
>>>>
>>>> return cc;
>>>> }
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_link.h b/drivers/gpu/drm/msm/dp/dp_link.h
>>>> index 9dd4dd9..fe8f716 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_link.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_link.h
>>>> @@ -12,6 +12,9 @@
>>>> #define DP_TEST_BIT_DEPTH_UNKNOWN 0xFFFFFFFF
>>>> #define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
>>>>
>>>> +#define DP_MISC0_LEGACY_RGB 0
>>>> +#define DP_MISC0_CEA_RGB 0x04
>>> These should go to dp_reg.h and should start with DP_MISC0_COLORIMETRY_CFG
>>>
>>>> +
>>>> struct dp_link_info {
>>>> unsigned char revision;
>>>> unsigned int rate;
>>>> --
>>>> 2.7.4
>>>>
>
>