On Thu, May 19, 2022 at 09:26:59AM -0700, Guillaume Ranquet wrote: > On Thu, 12 May 2022 09:44, Maxime Ripard <maxime@xxxxxxxxxx> 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:
signature.asc
Description: PGP signature