linux-kernel - Re: [PATCH v17 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <l27rc2lo4pt3mfwknjyy755li3q7vgqesoev25ulapwsenkkuj@w26ts47a2llz>
Date: Sat, 19 Oct 2024 18:02:01 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Maxime Ripard <mripard@...nel.org>
Cc: Sandor Yu <sandor.yu@....com>, 
	"andrzej.hajda@...el.com" <andrzej.hajda@...el.com>, "neil.armstrong@...aro.org" <neil.armstrong@...aro.org>, 
	Laurent Pinchart <laurent.pinchart@...asonboard.com>, "jonas@...boo.se" <jonas@...boo.se>, 
	"jernej.skrabec@...il.com" <jernej.skrabec@...il.com>, "airlied@...il.com" <airlied@...il.com>, 
	"daniel@...ll.ch" <daniel@...ll.ch>, "robh+dt@...nel.org" <robh+dt@...nel.org>, 
	"krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>, "shawnguo@...nel.org" <shawnguo@...nel.org>, 
	"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>, "festevam@...il.com" <festevam@...il.com>, 
	"vkoul@...nel.org" <vkoul@...nel.org>, 
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>, "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, 
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>, "kernel@...gutronix.de" <kernel@...gutronix.de>, 
	dl-linux-imx <linux-imx@....com>, Oliver Brown <oliver.brown@....com>, 
	"alexander.stein@...tq-group.com" <alexander.stein@...tq-group.com>, "sam@...nborg.org" <sam@...nborg.org>
Subject: Re: [PATCH v17 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

On Mon, Sep 30, 2024 at 10:18:06AM +0200, Maxime Ripard wrote:
> On Sun, Sep 29, 2024 at 02:34:36AM GMT, Sandor Yu wrote:
> > > > +static void cdns_hdmi_sink_config(struct cdns_mhdp8501_device *mhdp)
> > > > +{
> > > > +	struct drm_display_info *display = &mhdp->curr_conn->display_info;
> > > > +	struct drm_connector_state *conn_state = mhdp->curr_conn->state;
> > > 
> > > That looks a bit hackish to me. We should probably provide a helper to get the
> > > connector state the bridge is attached to.
> > 
> > How about code change to followed, is it more clear?
> > 370         struct drm_connector *connector = mhdp->curr_conn;
> > 371         struct drm_connector_state *conn_state = connector->state;
> > 372         struct drm_display_info *display = &connector->display_info;
> > 373         struct drm_scdc *scdc = &display->hdmi.scdc;
> 
> What I meant was that I wish bridges had a way to get their connector
> pointer. It doesn't look like it's possible with drm_bridge_connector,
> and we don't have access to drm_display_info anymore.
> 
> I don't really see a good way to do this yet, so maybe that kind of
> workaround is ok. Eventually, I guess we'll have the scrambler setup in
> the HDMI connector helpers anyway.
> 
> Dmitry, any idea?

Unfortunately nothing significant, I didn't have time to look at the
scrambler yet. The platforms that I'm working with either do not support
HDMI 2.0 or require no additional setup there.

Regarding the drm_connector_state, I had to go via the bridge->encoder,
then drm_atomic_get_new_connector_for_encoder() and finally
drm_atomic_get_new_connector_state(). I don't like the idea of storing
the connector in drm_bridge driver data, it seems like a bad idea to me.

> 
> > > > +static enum drm_mode_status
> > > > +cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
> > > > +			       const struct drm_display_mode *mode,
> > > > +			       unsigned long long tmds_rate) {
> > > > +	struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
> > > > +	union phy_configure_opts phy_cfg;
> > > > +	int ret;
> > > > +
> > > > +	phy_cfg.hdmi.tmds_char_rate = tmds_rate;
> > > > +
> > > > +	ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0, &phy_cfg);
> > > > +	if (ret < 0)
> > > > +		return MODE_CLOCK_RANGE;
> > > > +
> > > > +	return MODE_OK;
> > > > +}
> > > > +
> > > > +static enum drm_mode_status
> > > > +cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
> > > > +			    const struct drm_display_info *info,
> > > > +			    const struct drm_display_mode *mode) {
> > > > +	unsigned long long tmds_rate;
> > > > +
> > > > +	/* We don't support double-clocked and Interlaced modes */
> > > > +	if (mode->flags & DRM_MODE_FLAG_DBLCLK ||
> > > > +	    mode->flags & DRM_MODE_FLAG_INTERLACE)
> > > > +		return MODE_BAD;
> > > > +
> > > > +	if (mode->hdisplay > 3840)
> > > > +		return MODE_BAD_HVALUE;
> > > > +
> > > > +	if (mode->vdisplay > 2160)
> > > > +		return MODE_BAD_VVALUE;
> > > > +
> > > > +	tmds_rate = mode->clock * 1000ULL;
> > > > +	return cdns_hdmi_tmds_char_rate_valid(bridge, mode, tmds_rate); }
> > > 
> > > Didn't we agree on creating a mode_valid helper?
> > 
> > In fact, now I'm no idea where should add the mode_valid helper function.
> > 
> > In struct drm_bridge_funcs, it had mode_valid() and hdmi_tmds_char_rate_valid().
> > 
> > If create a new mode_valid helper function in struct drm_connector_hdmi_funcs,
> > Is it appropriate to call another API function(tmds_char_rate_valid)
> > at the same level within this API function?
> 
> I'm not quite sure what you mean, but a reasonable approach to me would
> be to turn drm_hdmi_state_helper.c hdmi_clock_valid into a public
> function, and then call it from drm_bridge_connector mode_valid hook.
> 
> It's a similar discussion to the previous one really: in order to
> implement it properly, we need access to drm_display_info.

I've sent a proposal, [1]. I don't think we should be using
hdmi_clock_valid() directly (at least not before sorting out the EDID /
hotplug handling in the HDMI Connector code) exactly because of the
info->max_tmds_clock. If it gets stale, we might filter modes
incorrectly. Also, I'm not sure if it should be left at 0 by default (or
in drm_parse_hdmi_forum_scds()).

[1] https://lore.kernel.org/dri-devel/20241018-hdmi-mode-valid-v1-0-6e49ae4801f7@linaro.org/

-- 
With best wishes
Dmitry