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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, 27 Jan 2024 at 05:57, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> On 1/26/2024 6:40 PM, Dmitry Baryshkov wrote:
> > On Sat, 27 Jan 2024 at 02:58, Paloma Arellano <quic_parellan@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >>>> ---
> >>>>    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
> >>>
> >
> >
> >



-- 
With best wishes
Dmitry



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux