[PATCH v2 19/19] drm/msm/dp: allow YUV420 mode for DP connector when CDM available
Abhinav Kumar
quic_abhinavk at quicinc.com
Tue Feb 13 00:18:17 UTC 2024
On 2/12/2024 1:20 PM, Dmitry Baryshkov wrote:
> On Mon, 12 Feb 2024 at 23:13, Abhinav Kumar <quic_abhinavk at 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 at 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 at 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 at 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.
More information about the dri-devel
mailing list