| Message ID | 20240125193834.7065-18-quic_parellan@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for CDM over DP | expand |
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; >
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; > >
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; >>> > > >
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; > >>> > > > > > >
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 --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;
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(-)