[PATCH 17/17] drm/msm/dp: allow YUV420 mode for DP connector when VSC SDP supported

[Dmitry Baryshkov]

On Mon, 29 Jan 2024 at 06:30, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>
>
>
> On 1/28/2024 7:52 PM, Dmitry Baryshkov wrote:
> > On Mon, 29 Jan 2024 at 05:17, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
> >>
> >>
> >>
> >> On 1/25/2024 2:05 PM, Dmitry Baryshkov wrote:
> >>> On 25/01/2024 21:38, Paloma Arellano wrote:
> >>>> All the components of YUV420 over DP are added. Therefore, let's mark the
> >>>> connector property as true for DP connector when the DP type is not eDP
> >>>> and when VSC SDP is supported.
> >>>>
> >>>> Signed-off-by: Paloma Arellano <quic_parellan at quicinc.com>
> >>>> ---
> >>>>    drivers/gpu/drm/msm/dp/dp_display.c | 5 ++++-
> >>>>    1 file changed, 4 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> index 4329435518351..97edd607400b8 100644
> >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >>>> @@ -370,11 +370,14 @@ static int dp_display_process_hpd_high(struct
> >>>> dp_display_private *dp)
> >>>>        dp_link_process_request(dp->link);
> >>>> -    if (!dp->dp_display.is_edp)
> >>>> +    if (!dp->dp_display.is_edp) {
> >>>> +        if (dp_panel_vsc_sdp_supported(dp->panel))
> >>>> +            dp->dp_display.connector->ycbcr_420_allowed = true;
> >>>
> >>> Please consider fixing a TODO in drm_bridge_connector_init().
> >>>
> >>
> >> I am not totally clear if that TODO can ever go for DP/HDMI usage of
> >> drm_bridge_connector.
> >>
> >> We do not know if the sink supports VSC SDP till we read the DPCD and
> >> till we know that sink supports VSC SDP, there is no reason to mark the
> >> YUV modes as supported. This is the same logic followed across vendors.
> >>
> >> drm_bride_connector_init() happens much earlier than the point where we
> >> read DPCD. The only thing which can be done is perhaps add some callback
> >> to update_ycbcr_420_allowed once DPCD is read. But I don't think its
> >> absolutely necessary to have a callback just for this.
> >
> > After checking the drm_connector docs, I'd still hold my opinion and
> > consider this patch to be a misuse of the property. If you check the
> > drm_connector::ycbcr_420_allowed docs, you'll see that it describes
> > the output from the source point of view. In other words, it should be
> > true if the DP connector can send YUV420 rather than being set if the
> > attached display supports such output. This matches ycbcr420_allowed
> > usage by AMD, dw-hdmi, intel_hdmi and even intel_dp usage.
> >
>
> hmmm I think I misread intel_dp_update_420(). I saw this is called after
> HPD so I thought they unset ycbcr_420_allowed if VSC SDP is not
> supported. But they have other DPCD checking there so anyway they will
> fail this bridge_connector_init() model.
>
> But one argument which I can give in my defense is, lets say the sink
> exposed YUV formats but did not support SDP, then atomic_check() will
> keep failing or should keep failing. This will avoid this scenario. But
> we can assume that would be a rogue sink.

This should be handled in DP's atomic_check. As usual, bonus point if
this is done via helpers that can be reused by other platforms.

> I think we can pass a yuv_supported flag to msm_dp_modeset_init() and
> set it to true from dpu_kms if catalog has CDM block and get rid of the
> dp_panel_vsc_sdp_supported().

These are two different issues. CDM should be checked in PDU (whether
the DPU can provide YUV data to the DP block).

>
> But that doesnt address the TODO you have pointed to. What is really the
> expectation of the TODO? Do we need to pass a ycbcr_420_allowed flag to
> drm_bridge_connector_init()?

Ugh. No. I was thinking about a `ycbcr420_allowed` flag in the struct
drm_bridge (to follow existing interlace_allowed) flag. But, this
might be not the best option. Each bridge can either pass through YUV
data from the previous bridge or generate YCbCr data on its own. So in
theory this demands two flags plus one flag for the encoder. Which
might be an overkill, until we end up in a situation when the driver
can not decide for the full bridge chain.

So let's probably ignore the TODO for the purpose of this series. Just
fix the usage of ycbcr420_allowed according to docs.

>
> That would need a tree wide cleanup and thats difficult to sign up for
> in this series and I would not as well.
>
> One thing which I can suggest to be less intrusive is have a new API
> called drm_bridge_connector_init_with_YUV() which looks something like
> below:
>
> struct drm_connector *drm_bridge_connector_init_with_ycbcr_420(struct
> drm_device *drm, struct drm_encoder *encoder)
> {
>         drm_bridge_connector_init();
>         connector->ycbcr_420_allowed = true;
> }
>
> But I don't know if the community would be interested in this idea or
> would find that useful.
>
> >>>>            drm_dp_set_subconnector_property(dp->dp_display.connector,
> >>>>                             connector_status_connected,
> >>>>                             dp->panel->dpcd,
> >>>>                             dp->panel->downstream_ports);
> >>>> +    }
> >>>>        edid = dp->panel->edid;
> >>>
> >
> >
> >



-- 
With best wishes
Dmitry