Received-SPF: pass (google.com: domain of linux-kernel+bounces-29317-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <33f9d97d-f0fb-96d7-eea9-c43c3a5a162e@quicinc.com>
Date: Wed, 17 Jan 2024 10:29:01 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v2] drm/msm/dp: correct configure Colorimetry Indicator
 Field at MISC0
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
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>
References: <1705514068-22394-1-git-send-email-quic_khsieh@quicinc.com>
 <CAA8EJpqO+SWYcCtH1hOVow18ZTbJ=qh0D68CEP1sGObdd3=ciA@mail.gmail.com>
Content-Language: en-US
From: Kuogee Hsieh <quic_khsieh@quicinc.com>
In-Reply-To: <CAA8EJpqO+SWYcCtH1hOVow18ZTbJ=qh0D68CEP1sGObdd3=ciA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


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