diff mbox series

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

Message ID 20240125193834.7065-18-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
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@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Abhinav Kumar Jan. 29, 2024, 3:17 a.m. UTC | #1
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@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.

>>           drm_dp_set_subconnector_property(dp->dp_display.connector,
>>                            connector_status_connected,
>>                            dp->panel->dpcd,
>>                            dp->panel->downstream_ports);
>> +    }
>>       edid = dp->panel->edid;
>
Dmitry Baryshkov Jan. 29, 2024, 3:52 a.m. UTC | #2
On Mon, 29 Jan 2024 at 05:17, Abhinav Kumar <quic_abhinavk@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@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.

> >>           drm_dp_set_subconnector_property(dp->dp_display.connector,
> >>                            connector_status_connected,
> >>                            dp->panel->dpcd,
> >>                            dp->panel->downstream_ports);
> >> +    }
> >>       edid = dp->panel->edid;
> >
Abhinav Kumar Jan. 29, 2024, 4:30 a.m. UTC | #3
On 1/28/2024 7:52 PM, Dmitry Baryshkov wrote:
> On Mon, 29 Jan 2024 at 05:17, Abhinav Kumar <quic_abhinavk@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@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;
>>>
> 
> 
>
Dmitry Baryshkov Jan. 29, 2024, 5:05 a.m. UTC | #4
On Mon, 29 Jan 2024 at 06:30, Abhinav Kumar <quic_abhinavk@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@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@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;
> >>>
> >
> >
> >
Abhinav Kumar Jan. 29, 2024, 5:36 a.m. UTC | #5
On 1/28/2024 9:05 PM, Dmitry Baryshkov wrote:
> On Mon, 29 Jan 2024 at 06:30, Abhinav Kumar <quic_abhinavk@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@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@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;
>>>>>
>>>
>>>
>>>
> 
> 
>
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 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;
 		drm_dp_set_subconnector_property(dp->dp_display.connector,
 						 connector_status_connected,
 						 dp->panel->dpcd,
 						 dp->panel->downstream_ports);
+	}
 
 	edid = dp->panel->edid;