| Message ID | 20240210015223.24670-20-quic_parellan@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for CDM over DP | expand |
On Sat, 10 Feb 2024 at 03:52, Paloma Arellano <quic_parellan@quicinc.com> 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 there is a CDM block available. > > Changes in v2: > - Check for if dp_catalog has a CDM block available instead of > checking if VSC SDP is allowed when setting the dp connector's > ycbcr_420_allowed parameter > > Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +++- > drivers/gpu/drm/msm/dp/dp_display.c | 4 ++-- > drivers/gpu/drm/msm/dp/dp_drm.c | 8 ++++++-- > drivers/gpu/drm/msm/dp/dp_drm.h | 3 ++- > drivers/gpu/drm/msm/msm_drv.h | 5 +++-- > 5 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index 723cc1d821431..beeaabe499abf 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -565,6 +565,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, > { > struct drm_encoder *encoder = NULL; > struct msm_display_info info; > + bool yuv_supported; > int rc; > int i; > > @@ -583,7 +584,8 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, > return PTR_ERR(encoder); > } > > - rc = msm_dp_modeset_init(priv->dp[i], dev, encoder); > + yuv_supported = !!(dpu_kms->catalog->cdm); Drop parentheses please. > + rc = msm_dp_modeset_init(priv->dp[i], dev, encoder, yuv_supported); > if (rc) { > DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc); > return rc; > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index ebcc76ef1d590..9b9f5f2921903 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -1471,7 +1471,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp) > } > > int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, > - struct drm_encoder *encoder) > + struct drm_encoder *encoder, bool yuv_supported) > { > struct dp_display_private *dp_priv; > int ret; > @@ -1487,7 +1487,7 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, > return ret; > } > > - dp_display->connector = dp_drm_connector_init(dp_display, encoder); > + dp_display->connector = dp_drm_connector_init(dp_display, encoder, yuv_supported); > if (IS_ERR(dp_display->connector)) { > ret = PTR_ERR(dp_display->connector); > DRM_DEV_ERROR(dev->dev, > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c > index 46e6889037e88..ab0d0d13b5e2c 100644 > --- a/drivers/gpu/drm/msm/dp/dp_drm.c > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c > @@ -353,7 +353,8 @@ int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, > } > > /* connector initialization */ > -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder) > +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder, > + bool yuv_supported) > { > struct drm_connector *connector = NULL; > > @@ -361,8 +362,11 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr > if (IS_ERR(connector)) > return connector; > > - if (!dp_display->is_edp) > + if (!dp_display->is_edp) { > drm_connector_attach_dp_subconnector_property(connector); > + if (yuv_supported) > + connector->ycbcr_420_allowed = true; Is there any reason to disallow YUV420 for eDP connectors? > + } > > drm_connector_attach_encoder(connector, encoder); > > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h > index b3d684db2383b..45e57ac25a4d9 100644 > --- a/drivers/gpu/drm/msm/dp/dp_drm.h > +++ b/drivers/gpu/drm/msm/dp/dp_drm.h > @@ -19,7 +19,8 @@ struct msm_dp_bridge { > > #define to_dp_bridge(x) container_of((x), struct msm_dp_bridge, bridge) > > -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder); > +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder, > + bool yuv_supported); > int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, > struct drm_encoder *encoder); > > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > index b876ebd48effe..37335777f5c09 100644 > --- a/drivers/gpu/drm/msm/msm_drv.h > +++ b/drivers/gpu/drm/msm/msm_drv.h > @@ -385,7 +385,7 @@ static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_ > int __init msm_dp_register(void); > void __exit msm_dp_unregister(void); > int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, > - struct drm_encoder *encoder); > + struct drm_encoder *encoder, bool yuv_supported); > void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display); > bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, > const struct drm_display_mode *mode); > @@ -403,7 +403,8 @@ static inline void __exit msm_dp_unregister(void) > } > static inline int msm_dp_modeset_init(struct msm_dp *dp_display, > struct drm_device *dev, > - struct drm_encoder *encoder) > + struct drm_encoder *encoder, > + bool yuv_supported) > { > return -EINVAL; > } > -- > 2.39.2 >
On 2/10/2024 3:33 AM, Dmitry Baryshkov wrote: > On Sat, 10 Feb 2024 at 03:52, Paloma Arellano <quic_parellan@quicinc.com> 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 there is a CDM block available. >> >> Changes in v2: >> - Check for if dp_catalog has a CDM block available instead of >> checking if VSC SDP is allowed when setting the dp connector's >> ycbcr_420_allowed parameter >> >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +++- >> drivers/gpu/drm/msm/dp/dp_display.c | 4 ++-- >> drivers/gpu/drm/msm/dp/dp_drm.c | 8 ++++++-- >> drivers/gpu/drm/msm/dp/dp_drm.h | 3 ++- >> drivers/gpu/drm/msm/msm_drv.h | 5 +++-- >> 5 files changed, 16 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> index 723cc1d821431..beeaabe499abf 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >> @@ -565,6 +565,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, >> { >> struct drm_encoder *encoder = NULL; >> struct msm_display_info info; >> + bool yuv_supported; >> int rc; >> int i; >> >> @@ -583,7 +584,8 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, >> return PTR_ERR(encoder); >> } >> >> - rc = msm_dp_modeset_init(priv->dp[i], dev, encoder); >> + yuv_supported = !!(dpu_kms->catalog->cdm); > > Drop parentheses please. > >> + rc = msm_dp_modeset_init(priv->dp[i], dev, encoder, yuv_supported); >> if (rc) { >> DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc); >> return rc; >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c >> index ebcc76ef1d590..9b9f5f2921903 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -1471,7 +1471,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp) >> } >> >> int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, >> - struct drm_encoder *encoder) >> + struct drm_encoder *encoder, bool yuv_supported) >> { >> struct dp_display_private *dp_priv; >> int ret; >> @@ -1487,7 +1487,7 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, >> return ret; >> } >> >> - dp_display->connector = dp_drm_connector_init(dp_display, encoder); >> + dp_display->connector = dp_drm_connector_init(dp_display, encoder, yuv_supported); >> if (IS_ERR(dp_display->connector)) { >> ret = PTR_ERR(dp_display->connector); >> DRM_DEV_ERROR(dev->dev, >> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c >> index 46e6889037e88..ab0d0d13b5e2c 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_drm.c >> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c >> @@ -353,7 +353,8 @@ int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, >> } >> >> /* connector initialization */ >> -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder) >> +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder, >> + bool yuv_supported) >> { >> struct drm_connector *connector = NULL; >> >> @@ -361,8 +362,11 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr >> if (IS_ERR(connector)) >> return connector; >> >> - if (!dp_display->is_edp) >> + if (!dp_display->is_edp) { >> drm_connector_attach_dp_subconnector_property(connector); >> + if (yuv_supported) >> + connector->ycbcr_420_allowed = true; > > Is there any reason to disallow YUV420 for eDP connectors? > >> + } >> Major reason was certainly validation but thinking about it more closely, I need to check whether CDM over eDP is even possible. Historically, CDM could output only to HDMI OR WB using the bit we program while setting up CDM and the same HDMI path is being used by DP as well. But I need to check whether CDM can even output to eDP with the same DP path. I dont have any documentation on this part yet.
On Sat, 10 Feb 2024 at 21:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 2/10/2024 3:33 AM, Dmitry Baryshkov wrote: > > On Sat, 10 Feb 2024 at 03:52, Paloma Arellano <quic_parellan@quicinc.com> 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 there is a CDM block available. > >> > >> Changes in v2: > >> - Check for if dp_catalog has a CDM block available instead of > >> checking if VSC SDP is allowed when setting the dp connector's > >> ycbcr_420_allowed parameter > >> > >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > >> --- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +++- > >> drivers/gpu/drm/msm/dp/dp_display.c | 4 ++-- > >> drivers/gpu/drm/msm/dp/dp_drm.c | 8 ++++++-- > >> drivers/gpu/drm/msm/dp/dp_drm.h | 3 ++- > >> drivers/gpu/drm/msm/msm_drv.h | 5 +++-- > >> 5 files changed, 16 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > >> index 723cc1d821431..beeaabe499abf 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > >> @@ -565,6 +565,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, > >> { > >> struct drm_encoder *encoder = NULL; > >> struct msm_display_info info; > >> + bool yuv_supported; > >> int rc; > >> int i; > >> > >> @@ -583,7 +584,8 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, > >> return PTR_ERR(encoder); > >> } > >> > >> - rc = msm_dp_modeset_init(priv->dp[i], dev, encoder); > >> + yuv_supported = !!(dpu_kms->catalog->cdm); > > > > Drop parentheses please. > > > >> + rc = msm_dp_modeset_init(priv->dp[i], dev, encoder, yuv_supported); > >> if (rc) { > >> DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc); > >> return rc; > >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > >> index ebcc76ef1d590..9b9f5f2921903 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_display.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c > >> @@ -1471,7 +1471,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp) > >> } > >> > >> int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, > >> - struct drm_encoder *encoder) > >> + struct drm_encoder *encoder, bool yuv_supported) > >> { > >> struct dp_display_private *dp_priv; > >> int ret; > >> @@ -1487,7 +1487,7 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, > >> return ret; > >> } > >> > >> - dp_display->connector = dp_drm_connector_init(dp_display, encoder); > >> + dp_display->connector = dp_drm_connector_init(dp_display, encoder, yuv_supported); > >> if (IS_ERR(dp_display->connector)) { > >> ret = PTR_ERR(dp_display->connector); > >> DRM_DEV_ERROR(dev->dev, > >> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c > >> index 46e6889037e88..ab0d0d13b5e2c 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_drm.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c > >> @@ -353,7 +353,8 @@ int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, > >> } > >> > >> /* connector initialization */ > >> -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder) > >> +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder, > >> + bool yuv_supported) > >> { > >> struct drm_connector *connector = NULL; > >> > >> @@ -361,8 +362,11 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr > >> if (IS_ERR(connector)) > >> return connector; > >> > >> - if (!dp_display->is_edp) > >> + if (!dp_display->is_edp) { > >> drm_connector_attach_dp_subconnector_property(connector); > >> + if (yuv_supported) > >> + connector->ycbcr_420_allowed = true; > > > > Is there any reason to disallow YUV420 for eDP connectors? > > > >> + } > >> > > Major reason was certainly validation but thinking about it more > closely, I need to check whether CDM over eDP is even possible. > > Historically, CDM could output only to HDMI OR WB using the bit we > program while setting up CDM and the same HDMI path is being used by DP > as well. But I need to check whether CDM can even output to eDP with the > same DP path. I dont have any documentation on this part yet. I had the feeling that the DP / eDP difference on the chips is mostly on the PHY and software side. So I assumed that it should be possible to output YUV data to the eDP port in the same way as it is done for the DP port.
On 2/10/2024 1:17 PM, Dmitry Baryshkov wrote: > On Sat, 10 Feb 2024 at 21:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 2/10/2024 3:33 AM, Dmitry Baryshkov wrote: >>> On Sat, 10 Feb 2024 at 03:52, Paloma Arellano <quic_parellan@quicinc.com> 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 there is a CDM block available. >>>> >>>> Changes in v2: >>>> - Check for if dp_catalog has a CDM block available instead of >>>> checking if VSC SDP is allowed when setting the dp connector's >>>> ycbcr_420_allowed parameter >>>> >>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>>> --- >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +++- >>>> drivers/gpu/drm/msm/dp/dp_display.c | 4 ++-- >>>> drivers/gpu/drm/msm/dp/dp_drm.c | 8 ++++++-- >>>> drivers/gpu/drm/msm/dp/dp_drm.h | 3 ++- >>>> drivers/gpu/drm/msm/msm_drv.h | 5 +++-- >>>> 5 files changed, 16 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>>> index 723cc1d821431..beeaabe499abf 100644 >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>>> @@ -565,6 +565,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, >>>> { >>>> struct drm_encoder *encoder = NULL; >>>> struct msm_display_info info; >>>> + bool yuv_supported; >>>> int rc; >>>> int i; >>>> >>>> @@ -583,7 +584,8 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, >>>> return PTR_ERR(encoder); >>>> } >>>> >>>> - rc = msm_dp_modeset_init(priv->dp[i], dev, encoder); >>>> + yuv_supported = !!(dpu_kms->catalog->cdm); >>> >>> Drop parentheses please. >>> >>>> + rc = msm_dp_modeset_init(priv->dp[i], dev, encoder, yuv_supported); >>>> if (rc) { >>>> DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc); >>>> return rc; >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c >>>> index ebcc76ef1d590..9b9f5f2921903 100644 >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>> @@ -1471,7 +1471,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp) >>>> } >>>> >>>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, >>>> - struct drm_encoder *encoder) >>>> + struct drm_encoder *encoder, bool yuv_supported) >>>> { >>>> struct dp_display_private *dp_priv; >>>> int ret; >>>> @@ -1487,7 +1487,7 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, >>>> return ret; >>>> } >>>> >>>> - dp_display->connector = dp_drm_connector_init(dp_display, encoder); >>>> + dp_display->connector = dp_drm_connector_init(dp_display, encoder, yuv_supported); >>>> if (IS_ERR(dp_display->connector)) { >>>> ret = PTR_ERR(dp_display->connector); >>>> DRM_DEV_ERROR(dev->dev, >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c >>>> index 46e6889037e88..ab0d0d13b5e2c 100644 >>>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c >>>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c >>>> @@ -353,7 +353,8 @@ int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, >>>> } >>>> >>>> /* connector initialization */ >>>> -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder) >>>> +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder, >>>> + bool yuv_supported) >>>> { >>>> struct drm_connector *connector = NULL; >>>> >>>> @@ -361,8 +362,11 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr >>>> if (IS_ERR(connector)) >>>> return connector; >>>> >>>> - if (!dp_display->is_edp) >>>> + if (!dp_display->is_edp) { >>>> drm_connector_attach_dp_subconnector_property(connector); >>>> + if (yuv_supported) >>>> + connector->ycbcr_420_allowed = true; >>> >>> Is there any reason to disallow YUV420 for eDP connectors? >>> >>>> + } >>>> >> >> Major reason was certainly validation but thinking about it more >> closely, I need to check whether CDM over eDP is even possible. >> >> Historically, CDM could output only to HDMI OR WB using the bit we >> program while setting up CDM and the same HDMI path is being used by DP >> as well. But I need to check whether CDM can even output to eDP with the >> same DP path. I dont have any documentation on this part yet. > > I had the feeling that the DP / eDP difference on the chips is mostly > on the PHY and software side. So I assumed that it should be possible > to output YUV data to the eDP port in the same way as it is done for > the DP port. > This is true. I was not worried about DP / eDP controller but mostly whether eDP spec really allows YUV. From what I can read, it does. So this check shall remain only because CDM has not been validated with eDP. Do you need a TODO here and if we ever get a eDP panel which supports that, we can validate and relax this.
On Mon, 12 Feb 2024 at 23:13, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 2/10/2024 1:17 PM, Dmitry Baryshkov wrote: > > On Sat, 10 Feb 2024 at 21:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> > >> > >> On 2/10/2024 3:33 AM, Dmitry Baryshkov wrote: > >>> On Sat, 10 Feb 2024 at 03:52, Paloma Arellano <quic_parellan@quicinc.com> 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 there is a CDM block available. > >>>> > >>>> Changes in v2: > >>>> - Check for if dp_catalog has a CDM block available instead of > >>>> checking if VSC SDP is allowed when setting the dp connector's > >>>> ycbcr_420_allowed parameter > >>>> > >>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > >>>> --- > >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +++- > >>>> drivers/gpu/drm/msm/dp/dp_display.c | 4 ++-- > >>>> drivers/gpu/drm/msm/dp/dp_drm.c | 8 ++++++-- > >>>> drivers/gpu/drm/msm/dp/dp_drm.h | 3 ++- > >>>> drivers/gpu/drm/msm/msm_drv.h | 5 +++-- > >>>> 5 files changed, 16 insertions(+), 8 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > >>>> index 723cc1d821431..beeaabe499abf 100644 > >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > >>>> @@ -565,6 +565,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, > >>>> { > >>>> struct drm_encoder *encoder = NULL; > >>>> struct msm_display_info info; > >>>> + bool yuv_supported; > >>>> int rc; > >>>> int i; > >>>> > >>>> @@ -583,7 +584,8 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, > >>>> return PTR_ERR(encoder); > >>>> } > >>>> > >>>> - rc = msm_dp_modeset_init(priv->dp[i], dev, encoder); > >>>> + yuv_supported = !!(dpu_kms->catalog->cdm); > >>> > >>> Drop parentheses please. > >>> > >>>> + rc = msm_dp_modeset_init(priv->dp[i], dev, encoder, yuv_supported); > >>>> if (rc) { > >>>> DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc); > >>>> return rc; > >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > >>>> index ebcc76ef1d590..9b9f5f2921903 100644 > >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c > >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c > >>>> @@ -1471,7 +1471,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp) > >>>> } > >>>> > >>>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, > >>>> - struct drm_encoder *encoder) > >>>> + struct drm_encoder *encoder, bool yuv_supported) > >>>> { > >>>> struct dp_display_private *dp_priv; > >>>> int ret; > >>>> @@ -1487,7 +1487,7 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, > >>>> return ret; > >>>> } > >>>> > >>>> - dp_display->connector = dp_drm_connector_init(dp_display, encoder); > >>>> + dp_display->connector = dp_drm_connector_init(dp_display, encoder, yuv_supported); > >>>> if (IS_ERR(dp_display->connector)) { > >>>> ret = PTR_ERR(dp_display->connector); > >>>> DRM_DEV_ERROR(dev->dev, > >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c > >>>> index 46e6889037e88..ab0d0d13b5e2c 100644 > >>>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c > >>>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c > >>>> @@ -353,7 +353,8 @@ int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, > >>>> } > >>>> > >>>> /* connector initialization */ > >>>> -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder) > >>>> +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder, > >>>> + bool yuv_supported) > >>>> { > >>>> struct drm_connector *connector = NULL; > >>>> > >>>> @@ -361,8 +362,11 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr > >>>> if (IS_ERR(connector)) > >>>> return connector; > >>>> > >>>> - if (!dp_display->is_edp) > >>>> + if (!dp_display->is_edp) { > >>>> drm_connector_attach_dp_subconnector_property(connector); > >>>> + if (yuv_supported) > >>>> + connector->ycbcr_420_allowed = true; > >>> > >>> Is there any reason to disallow YUV420 for eDP connectors? > >>> > >>>> + } > >>>> > >> > >> Major reason was certainly validation but thinking about it more > >> closely, I need to check whether CDM over eDP is even possible. > >> > >> Historically, CDM could output only to HDMI OR WB using the bit we > >> program while setting up CDM and the same HDMI path is being used by DP > >> as well. But I need to check whether CDM can even output to eDP with the > >> same DP path. I dont have any documentation on this part yet. > > > > I had the feeling that the DP / eDP difference on the chips is mostly > > on the PHY and software side. So I assumed that it should be possible > > to output YUV data to the eDP port in the same way as it is done for > > the DP port. > > > > This is true. I was not worried about DP / eDP controller but mostly > whether eDP spec really allows YUV. From what I can read, it does. > > So this check shall remain only because CDM has not been validated with eDP. > > Do you need a TODO here and if we ever get a eDP panel which supports > that, we can validate and relax this. Just move it out of the eDP check.
On 2/12/2024 1:20 PM, Dmitry Baryshkov wrote: > On Mon, 12 Feb 2024 at 23:13, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 2/10/2024 1:17 PM, Dmitry Baryshkov wrote: >>> On Sat, 10 Feb 2024 at 21:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 2/10/2024 3:33 AM, Dmitry Baryshkov wrote: >>>>> On Sat, 10 Feb 2024 at 03:52, Paloma Arellano <quic_parellan@quicinc.com> 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 there is a CDM block available. >>>>>> >>>>>> Changes in v2: >>>>>> - Check for if dp_catalog has a CDM block available instead of >>>>>> checking if VSC SDP is allowed when setting the dp connector's >>>>>> ycbcr_420_allowed parameter >>>>>> >>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>>>>> --- >>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +++- >>>>>> drivers/gpu/drm/msm/dp/dp_display.c | 4 ++-- >>>>>> drivers/gpu/drm/msm/dp/dp_drm.c | 8 ++++++-- >>>>>> drivers/gpu/drm/msm/dp/dp_drm.h | 3 ++- >>>>>> drivers/gpu/drm/msm/msm_drv.h | 5 +++-- >>>>>> 5 files changed, 16 insertions(+), 8 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>>>>> index 723cc1d821431..beeaabe499abf 100644 >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c >>>>>> @@ -565,6 +565,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, >>>>>> { >>>>>> struct drm_encoder *encoder = NULL; >>>>>> struct msm_display_info info; >>>>>> + bool yuv_supported; >>>>>> int rc; >>>>>> int i; >>>>>> >>>>>> @@ -583,7 +584,8 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, >>>>>> return PTR_ERR(encoder); >>>>>> } >>>>>> >>>>>> - rc = msm_dp_modeset_init(priv->dp[i], dev, encoder); >>>>>> + yuv_supported = !!(dpu_kms->catalog->cdm); >>>>> >>>>> Drop parentheses please. >>>>> >>>>>> + rc = msm_dp_modeset_init(priv->dp[i], dev, encoder, yuv_supported); >>>>>> if (rc) { >>>>>> DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc); >>>>>> return rc; >>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c >>>>>> index ebcc76ef1d590..9b9f5f2921903 100644 >>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>>>> @@ -1471,7 +1471,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp) >>>>>> } >>>>>> >>>>>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, >>>>>> - struct drm_encoder *encoder) >>>>>> + struct drm_encoder *encoder, bool yuv_supported) >>>>>> { >>>>>> struct dp_display_private *dp_priv; >>>>>> int ret; >>>>>> @@ -1487,7 +1487,7 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, >>>>>> return ret; >>>>>> } >>>>>> >>>>>> - dp_display->connector = dp_drm_connector_init(dp_display, encoder); >>>>>> + dp_display->connector = dp_drm_connector_init(dp_display, encoder, yuv_supported); >>>>>> if (IS_ERR(dp_display->connector)) { >>>>>> ret = PTR_ERR(dp_display->connector); >>>>>> DRM_DEV_ERROR(dev->dev, >>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c >>>>>> index 46e6889037e88..ab0d0d13b5e2c 100644 >>>>>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c >>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c >>>>>> @@ -353,7 +353,8 @@ int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, >>>>>> } >>>>>> >>>>>> /* connector initialization */ >>>>>> -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder) >>>>>> +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder, >>>>>> + bool yuv_supported) >>>>>> { >>>>>> struct drm_connector *connector = NULL; >>>>>> >>>>>> @@ -361,8 +362,11 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr >>>>>> if (IS_ERR(connector)) >>>>>> return connector; >>>>>> >>>>>> - if (!dp_display->is_edp) >>>>>> + if (!dp_display->is_edp) { >>>>>> drm_connector_attach_dp_subconnector_property(connector); >>>>>> + if (yuv_supported) >>>>>> + connector->ycbcr_420_allowed = true; >>>>> >>>>> Is there any reason to disallow YUV420 for eDP connectors? >>>>> >>>>>> + } >>>>>> >>>> >>>> Major reason was certainly validation but thinking about it more >>>> closely, I need to check whether CDM over eDP is even possible. >>>> >>>> Historically, CDM could output only to HDMI OR WB using the bit we >>>> program while setting up CDM and the same HDMI path is being used by DP >>>> as well. But I need to check whether CDM can even output to eDP with the >>>> same DP path. I dont have any documentation on this part yet. >>> >>> I had the feeling that the DP / eDP difference on the chips is mostly >>> on the PHY and software side. So I assumed that it should be possible >>> to output YUV data to the eDP port in the same way as it is done for >>> the DP port. >>> >> >> This is true. I was not worried about DP / eDP controller but mostly >> whether eDP spec really allows YUV. From what I can read, it does. >> >> So this check shall remain only because CDM has not been validated with eDP. >> >> Do you need a TODO here and if we ever get a eDP panel which supports >> that, we can validate and relax this. > > Just move it out of the eDP check. > I would have said a no to this because it opens a trap door for untested path which I usually hesitate but in this case, I am also curious to know if there is going to be a eDP panel to test this out for us because I have not seen any yet. So lets go ahead with the removal of !is_edp.
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 723cc1d821431..beeaabe499abf 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -565,6 +565,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, { struct drm_encoder *encoder = NULL; struct msm_display_info info; + bool yuv_supported; int rc; int i; @@ -583,7 +584,8 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, return PTR_ERR(encoder); } - rc = msm_dp_modeset_init(priv->dp[i], dev, encoder); + yuv_supported = !!(dpu_kms->catalog->cdm); + rc = msm_dp_modeset_init(priv->dp[i], dev, encoder, yuv_supported); if (rc) { DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc); return rc; diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index ebcc76ef1d590..9b9f5f2921903 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1471,7 +1471,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp) } int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, - struct drm_encoder *encoder) + struct drm_encoder *encoder, bool yuv_supported) { struct dp_display_private *dp_priv; int ret; @@ -1487,7 +1487,7 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, return ret; } - dp_display->connector = dp_drm_connector_init(dp_display, encoder); + dp_display->connector = dp_drm_connector_init(dp_display, encoder, yuv_supported); if (IS_ERR(dp_display->connector)) { ret = PTR_ERR(dp_display->connector); DRM_DEV_ERROR(dev->dev, diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index 46e6889037e88..ab0d0d13b5e2c 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -353,7 +353,8 @@ int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, } /* connector initialization */ -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder) +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder, + bool yuv_supported) { struct drm_connector *connector = NULL; @@ -361,8 +362,11 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr if (IS_ERR(connector)) return connector; - if (!dp_display->is_edp) + if (!dp_display->is_edp) { drm_connector_attach_dp_subconnector_property(connector); + if (yuv_supported) + connector->ycbcr_420_allowed = true; + } drm_connector_attach_encoder(connector, encoder); diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h index b3d684db2383b..45e57ac25a4d9 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.h +++ b/drivers/gpu/drm/msm/dp/dp_drm.h @@ -19,7 +19,8 @@ struct msm_dp_bridge { #define to_dp_bridge(x) container_of((x), struct msm_dp_bridge, bridge) -struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder); +struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder, + bool yuv_supported); int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, struct drm_encoder *encoder); diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index b876ebd48effe..37335777f5c09 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -385,7 +385,7 @@ static inline struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_ int __init msm_dp_register(void); void __exit msm_dp_unregister(void); int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, - struct drm_encoder *encoder); + struct drm_encoder *encoder, bool yuv_supported); void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display); bool msm_dp_is_yuv_420_enabled(const struct msm_dp *dp_display, const struct drm_display_mode *mode); @@ -403,7 +403,8 @@ static inline void __exit msm_dp_unregister(void) } static inline int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, - struct drm_encoder *encoder) + struct drm_encoder *encoder, + bool yuv_supported) { return -EINVAL; }
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 there is a CDM block available. Changes in v2: - Check for if dp_catalog has a CDM block available instead of checking if VSC SDP is allowed when setting the dp connector's ycbcr_420_allowed parameter Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 +++- drivers/gpu/drm/msm/dp/dp_display.c | 4 ++-- drivers/gpu/drm/msm/dp/dp_drm.c | 8 ++++++-- drivers/gpu/drm/msm/dp/dp_drm.h | 3 ++- drivers/gpu/drm/msm/msm_drv.h | 5 +++-- 5 files changed, 16 insertions(+), 8 deletions(-)