On Thu, 12 May 2022 09:44, Maxime Ripard maxime@cerno.tech wrote:
Hi,
On Wed, May 11, 2022 at 05:59:13AM -0700, Guillaume Ranquet wrote:
+#include <drm/drm_atomic_helper.h> +#include <drm/drm_bridge.h> +#include <drm/drm_crtc.h> +#include <drm/dp/drm_dp_helper.h> +#include <drm/drm_edid.h> +#include <drm/drm_of.h> +#include <drm/drm_panel.h> +#include <drm/drm_print.h> +#include <drm/drm_probe_helper.h> +#include <linux/arm-smccc.h> +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/errno.h> +#include <linux/kernel.h> +#include <linux/mfd/syscon.h> +#include <linux/nvmem-consumer.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/of_platform.h> +#include <linux/phy/phy.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <sound/hdmi-codec.h> +#include <video/videomode.h>
+#include "mtk_dp_reg.h"
+#define MTK_DP_AUX_WAIT_REPLY_COUNT 20 +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3
+//TODO: platform/device data or dts?
DTS :)
It's probably going to be a platform_data struct for v10... If I have time, I'll change it to a dts property for v10.
I can't really imagine a case where we would need platform_data nowadays. If you have a device tree, then it should be part of the binding.
What issue would you like to address by using a platform_data?
Ok, I'll migrate to dt then. I didn't realize platform_data were depreciated.
Angelo wants the MAX_LINRATE and MAX_LANES defines to be configurable. I imagined platform_data would be more appropriate as (per my understanding) the limitation is associated with a specific SoC.
+static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge) +{
- return connector_status_connected;
+}
I'm not quite sure what's going on there. You seem to have some support for HPD interrupts above, but you always report the display as connected?
I'd assume that either you don't have HPD support and then always report it as connected, or you have HPD support and report the current status in detect, but that combination seems weird.
The HPD logic needs more work, some things have been broken when I split the driver into three patches eDP - DP - Audio The assumption at first was that eDP didn't need any HPD handling... but it seems I was wrong and the eDP driver needs to be reworked.
That can be made into a patch of its own if you prefer.
You first introduce the driver without status reporting (always returning connected or unknown), and then add the needed bits for HPD.
However, that first patch shouldn't contain the interrupt plumbing and so on, it's just confusing.
After discussing a while with Mediatek, it appears that hot plug detection needs to be handled even for eDP... (which is always connected). So I'll revert to the split I made earlier in v8 where hot plug detection was part of the eDP patch the addition of the External display port was a trivial patch [1].
+static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
struct drm_connector *connector)+{
- struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
- bool enabled = mtk_dp->enabled;
- struct edid *new_edid = NULL;
- if (!enabled)
drm_bridge_chain_pre_enable(bridge);- drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
- usleep_range(2000, 5000);
- if (mtk_dp_plug_state(mtk_dp))
new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);- if (!enabled)
drm_bridge_chain_post_disable(bridge);Are you sure we can't get a mode set while get_edid is called?
If we can, then you could end up disabling the device while it's being powered on.
I'm a bit unsure, I need to spend more time in the drm stack to make sure. I'll get back to you when I have a definitive answer.
So, it looks like it's ok.
get_edid is your implementation of get_modes, which is called by drm_helper_probe_single_connector_modes
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_probe_hel...
This is the standard implemantion of fill_modes, which is called whenever the get_connector ioctl is called (or similar paths, like drm_client_modeset_probe)
drm_helper_probe_single_connector_modes is under the assumption that the mode_config.mutex is held though, and that the big lock. So it should be serialized there.
Just for future proofing though, it would be better to use refcounting there. Would runtime_pm work for you there?
Thx for looking into this for me. Not sure runtime_pm works here as it would only refcount if compiled with CONFIG_PM? I'd rather use the enabled field as a refcounter instead of a boolean.
Unless I'm totally missing your point?
Thx, Guillaume.
+static void mtk_dp_parse_drm_mode_timings(struct mtk_dp *mtk_dp,
struct drm_display_mode *mode)+{
- struct mtk_dp_timings *timings = &mtk_dp->info.timings;
- drm_display_mode_to_videomode(mode, &timings->vm);
- timings->frame_rate = mode->clock * 1000 / mode->htotal / mode->vtotal;
drm_mode_vrefresh()
- timings->htotal = mode->htotal;
- timings->vtotal = mode->vtotal;
+}
It's not really clear to me why you need to duplicate drm_display_mode here?
It's saved to be re-used in mtk_dp_set_msa(). It's not ideal, I'll check if I can get the mode directly from mtk_dp_set_msa()
Yeah, it looks like mtk_dp_set_msa() uses fairly straightforward values, this will be just as easy with drm_display_mode.
Maxime
[1] https://patchwork.kernel.org/project/linux-mediatek/patch/20220218145437.185...