Re: [PATCH v2] drm/msm/dp: correct configure Colorimetry Indicator Field at MISC0

[Kuogee Hsieh]

On 1/17/2024 10:12 AM, Dmitry Baryshkov wrote:
> On Wed, 17 Jan 2024 at 19:54, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > ---
> >   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