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