diff mbox series

[05/17] drm/msm/dp: add an API to indicate if sink supports VSC SDP

Message ID 20240125193834.7065-6-quic_parellan@quicinc.com
State New
Headers show
Series Add support for CDM over DP | expand

Commit Message

Paloma Arellano Jan. 25, 2024, 7:38 p.m. UTC
YUV420 format is supported only in the VSC SDP packet and not through
MSA. Hence add an API which indicates the sink support which can be used
by the rest of the DP programming.

Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c |  3 ++-
 drivers/gpu/drm/msm/dp/dp_panel.c   | 35 +++++++++++++++++++++++++----
 drivers/gpu/drm/msm/dp/dp_panel.h   |  1 +
 3 files changed, 34 insertions(+), 5 deletions(-)

Comments

Paloma Arellano Jan. 27, 2024, 12:58 a.m. UTC | #1
On 1/25/2024 1:23 PM, Dmitry Baryshkov wrote:
> On 25/01/2024 21:38, Paloma Arellano wrote:
>> YUV420 format is supported only in the VSC SDP packet and not through
>> MSA. Hence add an API which indicates the sink support which can be used
>> by the rest of the DP programming.
>
> This API ideally should go to drm/display/drm_dp_helper.c
I'm not familiar how other vendors are checking if VSC SDP is supported. 
So in moving this API, I'm going to let the other vendors make the 
changes themselves.
>
>>
>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dp/dp_display.c |  3 ++-
>>   drivers/gpu/drm/msm/dp/dp_panel.c   | 35 +++++++++++++++++++++++++----
>>   drivers/gpu/drm/msm/dp/dp_panel.h   |  1 +
>>   3 files changed, 34 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index ddac55f45a722..f6b3b6ca242f8 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1617,7 +1617,8 @@ void dp_bridge_mode_set(struct drm_bridge 
>> *drm_bridge,
>>           !!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC);
>>         dp_display->dp_mode.out_fmt_is_yuv_420 =
>> - drm_mode_is_420_only(&dp->connector->display_info, adjusted_mode);
>> + drm_mode_is_420_only(&dp->connector->display_info, adjusted_mode) &&
>> +        dp_panel_vsc_sdp_supported(dp_display->panel);
>>         /* populate wide_bus_support to different layers */
>>       dp_display->ctrl->wide_bus_en =
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c 
>> b/drivers/gpu/drm/msm/dp/dp_panel.c
>> index 127f6af995cd1..af7820b6d35ec 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>> @@ -17,6 +17,9 @@ struct dp_panel_private {
>>       struct dp_link *link;
>>       struct dp_catalog *catalog;
>>       bool panel_on;
>> +    bool vsc_supported;
>> +    u8 major;
>> +    u8 minor;
>>   };
>>     static void dp_panel_read_psr_cap(struct dp_panel_private *panel)
>> @@ -43,9 +46,10 @@ static void dp_panel_read_psr_cap(struct 
>> dp_panel_private *panel)
>>   static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
>>   {
>>       int rc;
>> +    ssize_t rlen;
>>       struct dp_panel_private *panel;
>>       struct dp_link_info *link_info;
>> -    u8 *dpcd, major, minor;
>> +    u8 *dpcd, rx_feature;
>>         panel = container_of(dp_panel, struct dp_panel_private, 
>> dp_panel);
>>       dpcd = dp_panel->dpcd;
>> @@ -53,10 +57,19 @@ static int dp_panel_read_dpcd(struct dp_panel 
>> *dp_panel)
>>       if (rc)
>>           return rc;
>>   +    rlen = drm_dp_dpcd_read(panel->aux, 
>> DP_DPRX_FEATURE_ENUMERATION_LIST, &rx_feature, 1);
>> +    if (rlen != 1) {
>> +        panel->vsc_supported = false;
>> +        pr_debug("failed to read DP_DPRX_FEATURE_ENUMERATION_LIST\n");
>> +    } else {
>> +        panel->vsc_supported = !!(rx_feature & 
>> DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED);
>> +        pr_debug("vsc=%d\n", panel->vsc_supported);
>> +    }
>> +
>>       link_info = &dp_panel->link_info;
>>       link_info->revision = dpcd[DP_DPCD_REV];
>> -    major = (link_info->revision >> 4) & 0x0f;
>> -    minor = link_info->revision & 0x0f;
>> +    panel->major = (link_info->revision >> 4) & 0x0f;
>> +    panel->minor = link_info->revision & 0x0f;
>>         link_info->rate = drm_dp_max_link_rate(dpcd);
>>       link_info->num_lanes = drm_dp_max_lane_count(dpcd);
>> @@ -69,7 +82,7 @@ static int dp_panel_read_dpcd(struct dp_panel 
>> *dp_panel)
>>       if (link_info->rate > dp_panel->max_dp_link_rate)
>>           link_info->rate = dp_panel->max_dp_link_rate;
>>   -    drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor);
>> +    drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", panel->major, 
>> panel->minor);
>>       drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate);
>>       drm_dbg_dp(panel->drm_dev, "lane_count=%d\n", 
>> link_info->num_lanes);
>>   @@ -280,6 +293,20 @@ void dp_panel_tpg_config(struct dp_panel 
>> *dp_panel, bool enable)
>>       dp_catalog_panel_tpg_enable(catalog, 
>> &panel->dp_panel.dp_mode.drm_mode);
>>   }
>>   +bool dp_panel_vsc_sdp_supported(struct dp_panel *dp_panel)
>> +{
>> +    struct dp_panel_private *panel;
>> +
>> +    if (!dp_panel) {
>> +        pr_err("invalid input\n");
>> +        return false;
>> +    }
>> +
>> +    panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
>> +
>> +    return panel->major >= 1 && panel->minor >= 3 && 
>> panel->vsc_supported;
>> +}
>> +
>>   void dp_panel_dump_regs(struct dp_panel *dp_panel)
>>   {
>>       struct dp_catalog *catalog;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h 
>> b/drivers/gpu/drm/msm/dp/dp_panel.h
>> index 6ec68be9f2366..590eca5ce304b 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
>> @@ -66,6 +66,7 @@ int dp_panel_get_modes(struct dp_panel *dp_panel,
>>           struct drm_connector *connector);
>>   void dp_panel_handle_sink_request(struct dp_panel *dp_panel);
>>   void dp_panel_tpg_config(struct dp_panel *dp_panel, bool enable);
>> +bool dp_panel_vsc_sdp_supported(struct dp_panel *dp_panel);
>>     /**
>>    * is_link_rate_valid() - validates the link rate
>
Dmitry Baryshkov Jan. 27, 2024, 2:40 a.m. UTC | #2
On Sat, 27 Jan 2024 at 02:58, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>
>
> On 1/25/2024 1:23 PM, Dmitry Baryshkov wrote:
> > On 25/01/2024 21:38, Paloma Arellano wrote:
> >> YUV420 format is supported only in the VSC SDP packet and not through
> >> MSA. Hence add an API which indicates the sink support which can be used
> >> by the rest of the DP programming.
> >
> > This API ideally should go to drm/display/drm_dp_helper.c
> I'm not familiar how other vendors are checking if VSC SDP is supported.
> So in moving this API, I'm going to let the other vendors make the
> changes themselves.

Let me show it for you:

bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp)
{
        u8 dprx = 0;

        if (drm_dp_dpcd_readb(&intel_dp->aux, DP_DPRX_FEATURE_ENUMERATION_LIST,
                              &dprx) != 1)
                return false;
        return dprx & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED;
}


> >
> >>
> >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/dp/dp_display.c |  3 ++-
> >>   drivers/gpu/drm/msm/dp/dp_panel.c   | 35 +++++++++++++++++++++++++----
> >>   drivers/gpu/drm/msm/dp/dp_panel.h   |  1 +
> >>   3 files changed, 34 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >> b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index ddac55f45a722..f6b3b6ca242f8 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -1617,7 +1617,8 @@ void dp_bridge_mode_set(struct drm_bridge
> >> *drm_bridge,
> >>           !!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC);
> >>         dp_display->dp_mode.out_fmt_is_yuv_420 =
> >> - drm_mode_is_420_only(&dp->connector->display_info, adjusted_mode);
> >> + drm_mode_is_420_only(&dp->connector->display_info, adjusted_mode) &&
> >> +        dp_panel_vsc_sdp_supported(dp_display->panel);
> >>         /* populate wide_bus_support to different layers */
> >>       dp_display->ctrl->wide_bus_en =
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
> >> b/drivers/gpu/drm/msm/dp/dp_panel.c
> >> index 127f6af995cd1..af7820b6d35ec 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> >> @@ -17,6 +17,9 @@ struct dp_panel_private {
> >>       struct dp_link *link;
> >>       struct dp_catalog *catalog;
> >>       bool panel_on;
> >> +    bool vsc_supported;
> >> +    u8 major;
> >> +    u8 minor;
> >>   };
> >>     static void dp_panel_read_psr_cap(struct dp_panel_private *panel)
> >> @@ -43,9 +46,10 @@ static void dp_panel_read_psr_cap(struct
> >> dp_panel_private *panel)
> >>   static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
> >>   {
> >>       int rc;
> >> +    ssize_t rlen;
> >>       struct dp_panel_private *panel;
> >>       struct dp_link_info *link_info;
> >> -    u8 *dpcd, major, minor;
> >> +    u8 *dpcd, rx_feature;
> >>         panel = container_of(dp_panel, struct dp_panel_private,
> >> dp_panel);
> >>       dpcd = dp_panel->dpcd;
> >> @@ -53,10 +57,19 @@ static int dp_panel_read_dpcd(struct dp_panel
> >> *dp_panel)
> >>       if (rc)
> >>           return rc;
> >>   +    rlen = drm_dp_dpcd_read(panel->aux,
> >> DP_DPRX_FEATURE_ENUMERATION_LIST, &rx_feature, 1);
> >> +    if (rlen != 1) {
> >> +        panel->vsc_supported = false;
> >> +        pr_debug("failed to read DP_DPRX_FEATURE_ENUMERATION_LIST\n");
> >> +    } else {
> >> +        panel->vsc_supported = !!(rx_feature &
> >> DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED);
> >> +        pr_debug("vsc=%d\n", panel->vsc_supported);
> >> +    }
> >> +
> >>       link_info = &dp_panel->link_info;
> >>       link_info->revision = dpcd[DP_DPCD_REV];
> >> -    major = (link_info->revision >> 4) & 0x0f;
> >> -    minor = link_info->revision & 0x0f;
> >> +    panel->major = (link_info->revision >> 4) & 0x0f;
> >> +    panel->minor = link_info->revision & 0x0f;
> >>         link_info->rate = drm_dp_max_link_rate(dpcd);
> >>       link_info->num_lanes = drm_dp_max_lane_count(dpcd);
> >> @@ -69,7 +82,7 @@ static int dp_panel_read_dpcd(struct dp_panel
> >> *dp_panel)
> >>       if (link_info->rate > dp_panel->max_dp_link_rate)
> >>           link_info->rate = dp_panel->max_dp_link_rate;
> >>   -    drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor);
> >> +    drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", panel->major,
> >> panel->minor);
> >>       drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate);
> >>       drm_dbg_dp(panel->drm_dev, "lane_count=%d\n",
> >> link_info->num_lanes);
> >>   @@ -280,6 +293,20 @@ void dp_panel_tpg_config(struct dp_panel
> >> *dp_panel, bool enable)
> >>       dp_catalog_panel_tpg_enable(catalog,
> >> &panel->dp_panel.dp_mode.drm_mode);
> >>   }
> >>   +bool dp_panel_vsc_sdp_supported(struct dp_panel *dp_panel)
> >> +{
> >> +    struct dp_panel_private *panel;
> >> +
> >> +    if (!dp_panel) {
> >> +        pr_err("invalid input\n");
> >> +        return false;
> >> +    }
> >> +
> >> +    panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
> >> +
> >> +    return panel->major >= 1 && panel->minor >= 3 &&
> >> panel->vsc_supported;

Anyway, this check is incorrect. Please compare the whole revision
against DP_DPCD_REV_13 instead of doing a maj/min comparison.

> >> +}
> >> +
> >>   void dp_panel_dump_regs(struct dp_panel *dp_panel)
> >>   {
> >>       struct dp_catalog *catalog;
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h
> >> b/drivers/gpu/drm/msm/dp/dp_panel.h
> >> index 6ec68be9f2366..590eca5ce304b 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
> >> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
> >> @@ -66,6 +66,7 @@ int dp_panel_get_modes(struct dp_panel *dp_panel,
> >>           struct drm_connector *connector);
> >>   void dp_panel_handle_sink_request(struct dp_panel *dp_panel);
> >>   void dp_panel_tpg_config(struct dp_panel *dp_panel, bool enable);
> >> +bool dp_panel_vsc_sdp_supported(struct dp_panel *dp_panel);
> >>     /**
> >>    * is_link_rate_valid() - validates the link rate
> >
Abhinav Kumar Jan. 27, 2024, 3:57 a.m. UTC | #3
On 1/26/2024 6:40 PM, Dmitry Baryshkov wrote:
> On Sat, 27 Jan 2024 at 02:58, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>>
>>
>> On 1/25/2024 1:23 PM, Dmitry Baryshkov wrote:
>>> On 25/01/2024 21:38, Paloma Arellano wrote:
>>>> YUV420 format is supported only in the VSC SDP packet and not through
>>>> MSA. Hence add an API which indicates the sink support which can be used
>>>> by the rest of the DP programming.
>>>
>>> This API ideally should go to drm/display/drm_dp_helper.c
>> I'm not familiar how other vendors are checking if VSC SDP is supported.
>> So in moving this API, I'm going to let the other vendors make the
>> changes themselves.
> 
> Let me show it for you:
> 
> bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp)
> {
>          u8 dprx = 0;
> 
>          if (drm_dp_dpcd_readb(&intel_dp->aux, DP_DPRX_FEATURE_ENUMERATION_LIST,
>                                &dprx) != 1)
>                  return false;
>          return dprx & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED;
> }
> 

Even AMD has similar logic:

6145 		stream->use_vsc_sdp_for_colorimetry = false;
6146 		if (aconnector->dc_sink->sink_signal == 
SIGNAL_TYPE_DISPLAY_PORT_MST) {
6147 			stream->use_vsc_sdp_for_colorimetry =
6148 				aconnector->dc_sink->is_vsc_sdp_colorimetry_supported;
6149 		} else {
6150 			if 
(stream->link->dpcd_caps.dprx_feature.bits.VSC_SDP_COLORIMETRY_SUPPORTED)
6151 				stream->use_vsc_sdp_for_colorimetry = true;
6152 		}

But it will be harder to untangle this compared to intel's code.

I am fine with adding an API to drm_dp_helper which indicates presence 
of VSC SDP but I would prefer leaving it to other vendors to use it in 
the way they would like and only keep msm usage in this series.



> 
>>>
>>>>
>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/dp/dp_display.c |  3 ++-
>>>>    drivers/gpu/drm/msm/dp/dp_panel.c   | 35 +++++++++++++++++++++++++----
>>>>    drivers/gpu/drm/msm/dp/dp_panel.h   |  1 +
>>>>    3 files changed, 34 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index ddac55f45a722..f6b3b6ca242f8 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -1617,7 +1617,8 @@ void dp_bridge_mode_set(struct drm_bridge
>>>> *drm_bridge,
>>>>            !!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC);
>>>>          dp_display->dp_mode.out_fmt_is_yuv_420 =
>>>> - drm_mode_is_420_only(&dp->connector->display_info, adjusted_mode);
>>>> + drm_mode_is_420_only(&dp->connector->display_info, adjusted_mode) &&
>>>> +        dp_panel_vsc_sdp_supported(dp_display->panel);
>>>>          /* populate wide_bus_support to different layers */
>>>>        dp_display->ctrl->wide_bus_en =
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> b/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> index 127f6af995cd1..af7820b6d35ec 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> @@ -17,6 +17,9 @@ struct dp_panel_private {
>>>>        struct dp_link *link;
>>>>        struct dp_catalog *catalog;
>>>>        bool panel_on;
>>>> +    bool vsc_supported;
>>>> +    u8 major;
>>>> +    u8 minor;
>>>>    };
>>>>      static void dp_panel_read_psr_cap(struct dp_panel_private *panel)
>>>> @@ -43,9 +46,10 @@ static void dp_panel_read_psr_cap(struct
>>>> dp_panel_private *panel)
>>>>    static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
>>>>    {
>>>>        int rc;
>>>> +    ssize_t rlen;
>>>>        struct dp_panel_private *panel;
>>>>        struct dp_link_info *link_info;
>>>> -    u8 *dpcd, major, minor;
>>>> +    u8 *dpcd, rx_feature;
>>>>          panel = container_of(dp_panel, struct dp_panel_private,
>>>> dp_panel);
>>>>        dpcd = dp_panel->dpcd;
>>>> @@ -53,10 +57,19 @@ static int dp_panel_read_dpcd(struct dp_panel
>>>> *dp_panel)
>>>>        if (rc)
>>>>            return rc;
>>>>    +    rlen = drm_dp_dpcd_read(panel->aux,
>>>> DP_DPRX_FEATURE_ENUMERATION_LIST, &rx_feature, 1);
>>>> +    if (rlen != 1) {
>>>> +        panel->vsc_supported = false;
>>>> +        pr_debug("failed to read DP_DPRX_FEATURE_ENUMERATION_LIST\n");
>>>> +    } else {
>>>> +        panel->vsc_supported = !!(rx_feature &
>>>> DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED);
>>>> +        pr_debug("vsc=%d\n", panel->vsc_supported);
>>>> +    }
>>>> +
>>>>        link_info = &dp_panel->link_info;
>>>>        link_info->revision = dpcd[DP_DPCD_REV];
>>>> -    major = (link_info->revision >> 4) & 0x0f;
>>>> -    minor = link_info->revision & 0x0f;
>>>> +    panel->major = (link_info->revision >> 4) & 0x0f;
>>>> +    panel->minor = link_info->revision & 0x0f;
>>>>          link_info->rate = drm_dp_max_link_rate(dpcd);
>>>>        link_info->num_lanes = drm_dp_max_lane_count(dpcd);
>>>> @@ -69,7 +82,7 @@ static int dp_panel_read_dpcd(struct dp_panel
>>>> *dp_panel)
>>>>        if (link_info->rate > dp_panel->max_dp_link_rate)
>>>>            link_info->rate = dp_panel->max_dp_link_rate;
>>>>    -    drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor);
>>>> +    drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", panel->major,
>>>> panel->minor);
>>>>        drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate);
>>>>        drm_dbg_dp(panel->drm_dev, "lane_count=%d\n",
>>>> link_info->num_lanes);
>>>>    @@ -280,6 +293,20 @@ void dp_panel_tpg_config(struct dp_panel
>>>> *dp_panel, bool enable)
>>>>        dp_catalog_panel_tpg_enable(catalog,
>>>> &panel->dp_panel.dp_mode.drm_mode);
>>>>    }
>>>>    +bool dp_panel_vsc_sdp_supported(struct dp_panel *dp_panel)
>>>> +{
>>>> +    struct dp_panel_private *panel;
>>>> +
>>>> +    if (!dp_panel) {
>>>> +        pr_err("invalid input\n");
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
>>>> +
>>>> +    return panel->major >= 1 && panel->minor >= 3 &&
>>>> panel->vsc_supported;
> 
> Anyway, this check is incorrect. Please compare the whole revision
> against DP_DPCD_REV_13 instead of doing a maj/min comparison.
> 
>>>> +}
>>>> +
>>>>    void dp_panel_dump_regs(struct dp_panel *dp_panel)
>>>>    {
>>>>        struct dp_catalog *catalog;
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> b/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> index 6ec68be9f2366..590eca5ce304b 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> @@ -66,6 +66,7 @@ int dp_panel_get_modes(struct dp_panel *dp_panel,
>>>>            struct drm_connector *connector);
>>>>    void dp_panel_handle_sink_request(struct dp_panel *dp_panel);
>>>>    void dp_panel_tpg_config(struct dp_panel *dp_panel, bool enable);
>>>> +bool dp_panel_vsc_sdp_supported(struct dp_panel *dp_panel);
>>>>      /**
>>>>     * is_link_rate_valid() - validates the link rate
>>>
> 
> 
>
Dmitry Baryshkov Jan. 27, 2024, 5:31 a.m. UTC | #4
On Sat, 27 Jan 2024 at 05:57, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 1/26/2024 6:40 PM, Dmitry Baryshkov wrote:
> > On Sat, 27 Jan 2024 at 02:58, Paloma Arellano <quic_parellan@quicinc.com> wrote:
> >>
> >>
> >> On 1/25/2024 1:23 PM, Dmitry Baryshkov wrote:
> >>> On 25/01/2024 21:38, Paloma Arellano wrote:
> >>>> YUV420 format is supported only in the VSC SDP packet and not through
> >>>> MSA. Hence add an API which indicates the sink support which can be used
> >>>> by the rest of the DP programming.
> >>>
> >>> This API ideally should go to drm/display/drm_dp_helper.c
> >> I'm not familiar how other vendors are checking if VSC SDP is supported.
> >> So in moving this API, I'm going to let the other vendors make the
> >> changes themselves.
> >
> > Let me show it for you:
> >
> > bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp)
> > {
> >          u8 dprx = 0;
> >
> >          if (drm_dp_dpcd_readb(&intel_dp->aux, DP_DPRX_FEATURE_ENUMERATION_LIST,
> >                                &dprx) != 1)
> >                  return false;
> >          return dprx & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED;
> > }
> >
>
> Even AMD has similar logic:
>
> 6145            stream->use_vsc_sdp_for_colorimetry = false;
> 6146            if (aconnector->dc_sink->sink_signal ==
> SIGNAL_TYPE_DISPLAY_PORT_MST) {
> 6147                    stream->use_vsc_sdp_for_colorimetry =
> 6148                            aconnector->dc_sink->is_vsc_sdp_colorimetry_supported;
> 6149            } else {
> 6150                    if
> (stream->link->dpcd_caps.dprx_feature.bits.VSC_SDP_COLORIMETRY_SUPPORTED)
> 6151                            stream->use_vsc_sdp_for_colorimetry = true;
> 6152            }
>
> But it will be harder to untangle this compared to intel's code.
>
> I am fine with adding an API to drm_dp_helper which indicates presence
> of VSC SDP but I would prefer leaving it to other vendors to use it in
> the way they would like and only keep msm usage in this series.

SGTM

>
>
>
> >
> >>>
> >>>>
> >>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
> >>>> ---
> >>>>    drivers/gpu/drm/msm/dp/dp_display.c |  3 ++-
> >>>>    drivers/gpu/drm/msm/dp/dp_panel.c   | 35 +++++++++++++++++++++++++----
> >>>>    drivers/gpu/drm/msm/dp/dp_panel.h   |  1 +
> >>>>    3 files changed, 34 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> index ddac55f45a722..f6b3b6ca242f8 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> @@ -1617,7 +1617,8 @@ void dp_bridge_mode_set(struct drm_bridge
> >>>> *drm_bridge,
> >>>>            !!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC);
> >>>>          dp_display->dp_mode.out_fmt_is_yuv_420 =
> >>>> - drm_mode_is_420_only(&dp->connector->display_info, adjusted_mode);
> >>>> + drm_mode_is_420_only(&dp->connector->display_info, adjusted_mode) &&
> >>>> +        dp_panel_vsc_sdp_supported(dp_display->panel);
> >>>>          /* populate wide_bus_support to different layers */
> >>>>        dp_display->ctrl->wide_bus_en =
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
> >>>> b/drivers/gpu/drm/msm/dp/dp_panel.c
> >>>> index 127f6af995cd1..af7820b6d35ec 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> >>>> @@ -17,6 +17,9 @@ struct dp_panel_private {
> >>>>        struct dp_link *link;
> >>>>        struct dp_catalog *catalog;
> >>>>        bool panel_on;
> >>>> +    bool vsc_supported;
> >>>> +    u8 major;
> >>>> +    u8 minor;
> >>>>    };
> >>>>      static void dp_panel_read_psr_cap(struct dp_panel_private *panel)
> >>>> @@ -43,9 +46,10 @@ static void dp_panel_read_psr_cap(struct
> >>>> dp_panel_private *panel)
> >>>>    static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
> >>>>    {
> >>>>        int rc;
> >>>> +    ssize_t rlen;
> >>>>        struct dp_panel_private *panel;
> >>>>        struct dp_link_info *link_info;
> >>>> -    u8 *dpcd, major, minor;
> >>>> +    u8 *dpcd, rx_feature;
> >>>>          panel = container_of(dp_panel, struct dp_panel_private,
> >>>> dp_panel);
> >>>>        dpcd = dp_panel->dpcd;
> >>>> @@ -53,10 +57,19 @@ static int dp_panel_read_dpcd(struct dp_panel
> >>>> *dp_panel)
> >>>>        if (rc)
> >>>>            return rc;
> >>>>    +    rlen = drm_dp_dpcd_read(panel->aux,
> >>>> DP_DPRX_FEATURE_ENUMERATION_LIST, &rx_feature, 1);
> >>>> +    if (rlen != 1) {
> >>>> +        panel->vsc_supported = false;
> >>>> +        pr_debug("failed to read DP_DPRX_FEATURE_ENUMERATION_LIST\n");
> >>>> +    } else {
> >>>> +        panel->vsc_supported = !!(rx_feature &
> >>>> DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED);
> >>>> +        pr_debug("vsc=%d\n", panel->vsc_supported);
> >>>> +    }
> >>>> +
> >>>>        link_info = &dp_panel->link_info;
> >>>>        link_info->revision = dpcd[DP_DPCD_REV];
> >>>> -    major = (link_info->revision >> 4) & 0x0f;
> >>>> -    minor = link_info->revision & 0x0f;
> >>>> +    panel->major = (link_info->revision >> 4) & 0x0f;
> >>>> +    panel->minor = link_info->revision & 0x0f;
> >>>>          link_info->rate = drm_dp_max_link_rate(dpcd);
> >>>>        link_info->num_lanes = drm_dp_max_lane_count(dpcd);
> >>>> @@ -69,7 +82,7 @@ static int dp_panel_read_dpcd(struct dp_panel
> >>>> *dp_panel)
> >>>>        if (link_info->rate > dp_panel->max_dp_link_rate)
> >>>>            link_info->rate = dp_panel->max_dp_link_rate;
> >>>>    -    drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor);
> >>>> +    drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", panel->major,
> >>>> panel->minor);
> >>>>        drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate);
> >>>>        drm_dbg_dp(panel->drm_dev, "lane_count=%d\n",
> >>>> link_info->num_lanes);
> >>>>    @@ -280,6 +293,20 @@ void dp_panel_tpg_config(struct dp_panel
> >>>> *dp_panel, bool enable)
> >>>>        dp_catalog_panel_tpg_enable(catalog,
> >>>> &panel->dp_panel.dp_mode.drm_mode);
> >>>>    }
> >>>>    +bool dp_panel_vsc_sdp_supported(struct dp_panel *dp_panel)
> >>>> +{
> >>>> +    struct dp_panel_private *panel;
> >>>> +
> >>>> +    if (!dp_panel) {
> >>>> +        pr_err("invalid input\n");
> >>>> +        return false;
> >>>> +    }
> >>>> +
> >>>> +    panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
> >>>> +
> >>>> +    return panel->major >= 1 && panel->minor >= 3 &&
> >>>> panel->vsc_supported;
> >
> > Anyway, this check is incorrect. Please compare the whole revision
> > against DP_DPCD_REV_13 instead of doing a maj/min comparison.
> >
> >>>> +}
> >>>> +
> >>>>    void dp_panel_dump_regs(struct dp_panel *dp_panel)
> >>>>    {
> >>>>        struct dp_catalog *catalog;
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h
> >>>> b/drivers/gpu/drm/msm/dp/dp_panel.h
> >>>> index 6ec68be9f2366..590eca5ce304b 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
> >>>> @@ -66,6 +66,7 @@ int dp_panel_get_modes(struct dp_panel *dp_panel,
> >>>>            struct drm_connector *connector);
> >>>>    void dp_panel_handle_sink_request(struct dp_panel *dp_panel);
> >>>>    void dp_panel_tpg_config(struct dp_panel *dp_panel, bool enable);
> >>>> +bool dp_panel_vsc_sdp_supported(struct dp_panel *dp_panel);
> >>>>      /**
> >>>>     * is_link_rate_valid() - validates the link rate
> >>>
> >
> >
> >
Paloma Arellano Jan. 29, 2024, 11:20 p.m. UTC | #5
On 1/26/2024 6:40 PM, Dmitry Baryshkov wrote:
> On Sat, 27 Jan 2024 at 02:58, Paloma Arellano <quic_parellan@quicinc.com> wrote:
>>
>> On 1/25/2024 1:23 PM, Dmitry Baryshkov wrote:
>>> On 25/01/2024 21:38, Paloma Arellano wrote:
>>>> YUV420 format is supported only in the VSC SDP packet and not through
>>>> MSA. Hence add an API which indicates the sink support which can be used
>>>> by the rest of the DP programming.
>>> This API ideally should go to drm/display/drm_dp_helper.c
>> I'm not familiar how other vendors are checking if VSC SDP is supported.
>> So in moving this API, I'm going to let the other vendors make the
>> changes themselves.
> Let me show it for you:
>
> bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp)
> {
>          u8 dprx = 0;
>
>          if (drm_dp_dpcd_readb(&intel_dp->aux, DP_DPRX_FEATURE_ENUMERATION_LIST,
>                                &dprx) != 1)
>                  return false;
>          return dprx & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED;
> }
>
>
>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/dp/dp_display.c |  3 ++-
>>>>    drivers/gpu/drm/msm/dp/dp_panel.c   | 35 +++++++++++++++++++++++++----
>>>>    drivers/gpu/drm/msm/dp/dp_panel.h   |  1 +
>>>>    3 files changed, 34 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index ddac55f45a722..f6b3b6ca242f8 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -1617,7 +1617,8 @@ void dp_bridge_mode_set(struct drm_bridge
>>>> *drm_bridge,
>>>>            !!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC);
>>>>          dp_display->dp_mode.out_fmt_is_yuv_420 =
>>>> - drm_mode_is_420_only(&dp->connector->display_info, adjusted_mode);
>>>> + drm_mode_is_420_only(&dp->connector->display_info, adjusted_mode) &&
>>>> +        dp_panel_vsc_sdp_supported(dp_display->panel);
>>>>          /* populate wide_bus_support to different layers */
>>>>        dp_display->ctrl->wide_bus_en =
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> b/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> index 127f6af995cd1..af7820b6d35ec 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> @@ -17,6 +17,9 @@ struct dp_panel_private {
>>>>        struct dp_link *link;
>>>>        struct dp_catalog *catalog;
>>>>        bool panel_on;
>>>> +    bool vsc_supported;
>>>> +    u8 major;
>>>> +    u8 minor;
>>>>    };
>>>>      static void dp_panel_read_psr_cap(struct dp_panel_private *panel)
>>>> @@ -43,9 +46,10 @@ static void dp_panel_read_psr_cap(struct
>>>> dp_panel_private *panel)
>>>>    static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
>>>>    {
>>>>        int rc;
>>>> +    ssize_t rlen;
>>>>        struct dp_panel_private *panel;
>>>>        struct dp_link_info *link_info;
>>>> -    u8 *dpcd, major, minor;
>>>> +    u8 *dpcd, rx_feature;
>>>>          panel = container_of(dp_panel, struct dp_panel_private,
>>>> dp_panel);
>>>>        dpcd = dp_panel->dpcd;
>>>> @@ -53,10 +57,19 @@ static int dp_panel_read_dpcd(struct dp_panel
>>>> *dp_panel)
>>>>        if (rc)
>>>>            return rc;
>>>>    +    rlen = drm_dp_dpcd_read(panel->aux,
>>>> DP_DPRX_FEATURE_ENUMERATION_LIST, &rx_feature, 1);
>>>> +    if (rlen != 1) {
>>>> +        panel->vsc_supported = false;
>>>> +        pr_debug("failed to read DP_DPRX_FEATURE_ENUMERATION_LIST\n");
>>>> +    } else {
>>>> +        panel->vsc_supported = !!(rx_feature &
>>>> DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED);
>>>> +        pr_debug("vsc=%d\n", panel->vsc_supported);
>>>> +    }
>>>> +
>>>>        link_info = &dp_panel->link_info;
>>>>        link_info->revision = dpcd[DP_DPCD_REV];
>>>> -    major = (link_info->revision >> 4) & 0x0f;
>>>> -    minor = link_info->revision & 0x0f;
>>>> +    panel->major = (link_info->revision >> 4) & 0x0f;
>>>> +    panel->minor = link_info->revision & 0x0f;
>>>>          link_info->rate = drm_dp_max_link_rate(dpcd);
>>>>        link_info->num_lanes = drm_dp_max_lane_count(dpcd);
>>>> @@ -69,7 +82,7 @@ static int dp_panel_read_dpcd(struct dp_panel
>>>> *dp_panel)
>>>>        if (link_info->rate > dp_panel->max_dp_link_rate)
>>>>            link_info->rate = dp_panel->max_dp_link_rate;
>>>>    -    drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor);
>>>> +    drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", panel->major,
>>>> panel->minor);
>>>>        drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate);
>>>>        drm_dbg_dp(panel->drm_dev, "lane_count=%d\n",
>>>> link_info->num_lanes);
>>>>    @@ -280,6 +293,20 @@ void dp_panel_tpg_config(struct dp_panel
>>>> *dp_panel, bool enable)
>>>>        dp_catalog_panel_tpg_enable(catalog,
>>>> &panel->dp_panel.dp_mode.drm_mode);
>>>>    }
>>>>    +bool dp_panel_vsc_sdp_supported(struct dp_panel *dp_panel)
>>>> +{
>>>> +    struct dp_panel_private *panel;
>>>> +
>>>> +    if (!dp_panel) {
>>>> +        pr_err("invalid input\n");
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
>>>> +
>>>> +    return panel->major >= 1 && panel->minor >= 3 &&
>>>> panel->vsc_supported;
> Anyway, this check is incorrect. Please compare the whole revision
> against DP_DPCD_REV_13 instead of doing a maj/min comparison.
Ack
>
>>>> +}
>>>> +
>>>>    void dp_panel_dump_regs(struct dp_panel *dp_panel)
>>>>    {
>>>>        struct dp_catalog *catalog;
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> b/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> index 6ec68be9f2366..590eca5ce304b 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
>>>> @@ -66,6 +66,7 @@ int dp_panel_get_modes(struct dp_panel *dp_panel,
>>>>            struct drm_connector *connector);
>>>>    void dp_panel_handle_sink_request(struct dp_panel *dp_panel);
>>>>    void dp_panel_tpg_config(struct dp_panel *dp_panel, bool enable);
>>>> +bool dp_panel_vsc_sdp_supported(struct dp_panel *dp_panel);
>>>>      /**
>>>>     * is_link_rate_valid() - validates the link rate
>
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index ddac55f45a722..f6b3b6ca242f8 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1617,7 +1617,8 @@  void dp_bridge_mode_set(struct drm_bridge *drm_bridge,
 		!!(dp_display->dp_mode.drm_mode.flags & DRM_MODE_FLAG_NHSYNC);
 
 	dp_display->dp_mode.out_fmt_is_yuv_420 =
-		drm_mode_is_420_only(&dp->connector->display_info, adjusted_mode);
+		drm_mode_is_420_only(&dp->connector->display_info, adjusted_mode) &&
+		dp_panel_vsc_sdp_supported(dp_display->panel);
 
 	/* populate wide_bus_support to different layers */
 	dp_display->ctrl->wide_bus_en =
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index 127f6af995cd1..af7820b6d35ec 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -17,6 +17,9 @@  struct dp_panel_private {
 	struct dp_link *link;
 	struct dp_catalog *catalog;
 	bool panel_on;
+	bool vsc_supported;
+	u8 major;
+	u8 minor;
 };
 
 static void dp_panel_read_psr_cap(struct dp_panel_private *panel)
@@ -43,9 +46,10 @@  static void dp_panel_read_psr_cap(struct dp_panel_private *panel)
 static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
 {
 	int rc;
+	ssize_t rlen;
 	struct dp_panel_private *panel;
 	struct dp_link_info *link_info;
-	u8 *dpcd, major, minor;
+	u8 *dpcd, rx_feature;
 
 	panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
 	dpcd = dp_panel->dpcd;
@@ -53,10 +57,19 @@  static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
 	if (rc)
 		return rc;
 
+	rlen = drm_dp_dpcd_read(panel->aux, DP_DPRX_FEATURE_ENUMERATION_LIST, &rx_feature, 1);
+	if (rlen != 1) {
+		panel->vsc_supported = false;
+		pr_debug("failed to read DP_DPRX_FEATURE_ENUMERATION_LIST\n");
+	} else {
+		panel->vsc_supported = !!(rx_feature & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED);
+		pr_debug("vsc=%d\n", panel->vsc_supported);
+	}
+
 	link_info = &dp_panel->link_info;
 	link_info->revision = dpcd[DP_DPCD_REV];
-	major = (link_info->revision >> 4) & 0x0f;
-	minor = link_info->revision & 0x0f;
+	panel->major = (link_info->revision >> 4) & 0x0f;
+	panel->minor = link_info->revision & 0x0f;
 
 	link_info->rate = drm_dp_max_link_rate(dpcd);
 	link_info->num_lanes = drm_dp_max_lane_count(dpcd);
@@ -69,7 +82,7 @@  static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
 	if (link_info->rate > dp_panel->max_dp_link_rate)
 		link_info->rate = dp_panel->max_dp_link_rate;
 
-	drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", major, minor);
+	drm_dbg_dp(panel->drm_dev, "version: %d.%d\n", panel->major, panel->minor);
 	drm_dbg_dp(panel->drm_dev, "link_rate=%d\n", link_info->rate);
 	drm_dbg_dp(panel->drm_dev, "lane_count=%d\n", link_info->num_lanes);
 
@@ -280,6 +293,20 @@  void dp_panel_tpg_config(struct dp_panel *dp_panel, bool enable)
 	dp_catalog_panel_tpg_enable(catalog, &panel->dp_panel.dp_mode.drm_mode);
 }
 
+bool dp_panel_vsc_sdp_supported(struct dp_panel *dp_panel)
+{
+	struct dp_panel_private *panel;
+
+	if (!dp_panel) {
+		pr_err("invalid input\n");
+		return false;
+	}
+
+	panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
+
+	return panel->major >= 1 && panel->minor >= 3 && panel->vsc_supported;
+}
+
 void dp_panel_dump_regs(struct dp_panel *dp_panel)
 {
 	struct dp_catalog *catalog;
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
index 6ec68be9f2366..590eca5ce304b 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -66,6 +66,7 @@  int dp_panel_get_modes(struct dp_panel *dp_panel,
 		struct drm_connector *connector);
 void dp_panel_handle_sink_request(struct dp_panel *dp_panel);
 void dp_panel_tpg_config(struct dp_panel *dp_panel, bool enable);
+bool dp_panel_vsc_sdp_supported(struct dp_panel *dp_panel);
 
 /**
  * is_link_rate_valid() - validates the link rate