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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 1/28/2024 7:52 PM, Dmitry Baryshkov wrote:
On Mon, 29 Jan 2024 at 05:17, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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@xxxxxxxxxxx>
---
   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;








[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux