Re: [PATCH v9 18/22] drm/mediatek: Add mt8195 Embedded DisplayPort driver

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 4399 bytes --]

On Thu, May 19, 2022 at 09:26:59AM -0700, Guillaume Ranquet wrote:
> 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.

The entire device tree is nothing but a collection of data associated to
a specific SoC though :)

> >> >> +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_helper.c#L416
> >
> >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?

It should be enabled in most configurations these days, and you can
always depend on it in your Kconfig option.

> I'd rather use the enabled field as a refcounter instead of a boolean.

It's a bit more ad-hoc, but that would work too. Make sure to use a lock
or atomic operations though

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]