Received-SPF: pass (google.com: domain of linux-kernel+bounces-29327-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <1705514068-22394-1-git-send-email-quic_khsieh@quicinc.com>
 <CAA8EJpqO+SWYcCtH1hOVow18ZTbJ=qh0D68CEP1sGObdd3=ciA@mail.gmail.com> <33f9d97d-f0fb-96d7-eea9-c43c3a5a162e@quicinc.com>
In-Reply-To: <33f9d97d-f0fb-96d7-eea9-c43c3a5a162e@quicinc.com>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Wed, 17 Jan 2024 20:40:34 +0200
Message-ID: <CAA8EJppWuUrjzmssh79JT3B4uDcc_WnKGhJoCNiG8NTw9xD6wg@mail.gmail.com>
Subject: Re: [PATCH v2] drm/msm/dp: correct configure Colorimetry Indicator
 Field at MISC0
To: Kuogee Hsieh <quic_khsieh@quicinc.com>
Cc: dri-devel@lists.freedesktop.org, robdclark@gmail.com, sean@poorly.run, 
	swboyd@chromium.org, dianders@chromium.org, vkoul@kernel.org, daniel@ffwll.ch, 
	airlied@gmail.com, agross@kernel.org, andersson@kernel.org, 
	quic_abhinavk@quicinc.com, quic_jesszhan@quicinc.com, 
	quic_sbillaka@quicinc.com, marijn.suijten@somainline.org, 
	freedreno@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

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
> >>
> >


-- 
With best wishes
Dmitry