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

[Abhinav Kumar]

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.

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().

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()?

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