| Message ID | 1705514068-22394-1-git-send-email-quic_khsieh@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2] drm/msm/dp: correct configure Colorimetry Indicator Field at MISC0 | expand |
On Wed, 17 Jan 2024 at 19:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 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 <quic_khsieh@quicinc.com> > --- > 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 1/17/2024 10:12 AM, Dmitry Baryshkov wrote: > On Wed, 17 Jan 2024 at 19:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 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 <quic_khsieh@quicinc.com> >> --- >> 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 <quic_khsieh@quicinc.com> wrote: > > > On 1/17/2024 10:12 AM, Dmitry Baryshkov wrote: > > On Wed, 17 Jan 2024 at 19:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 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 <quic_khsieh@quicinc.com> > >> --- > >> 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 1/17/2024 10:40 AM, Dmitry Baryshkov wrote: > On Wed, 17 Jan 2024 at 20:29, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: >> >> On 1/17/2024 10:12 AM, Dmitry Baryshkov wrote: >>> On Wed, 17 Jan 2024 at 19:54, Kuogee Hsieh <quic_khsieh@quicinc.com> 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 <quic_khsieh@quicinc.com> >>>> --- >>>> 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 >>>> > >
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;
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 <quic_khsieh@quicinc.com> --- 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(-)