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

[Abhinav Kumar]

On 1/28/2024 9:05 PM, Dmitry Baryshkov wrote:
> 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).
> 

Yes, I found this issue while discussing this. We need to make this change.

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

Yes.

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

Ack.

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