Message-ID: <20220512074446.ihilbbnbuwesxbbg@houat>
Date:   Thu, 12 May 2022 09:44:46 +0200
From:   Maxime Ripard <maxime@...no.tech>
To:     Guillaume Ranquet <granquet@...libre.com>
Cc:     airlied@...ux.ie, angelogioacchino.delregno@...labora.com,
        chunfeng.yun@...iatek.com, chunkuang.hu@...nel.org,
        ck.hu@...iatek.com, daniel@...ll.ch, deller@....de,
        jitao.shi@...iatek.com, kishon@...com, krzk+dt@...nel.org,
        maarten.lankhorst@...ux.intel.com, matthias.bgg@...il.com,
        p.zabel@...gutronix.de, robh+dt@...nel.org, tzimmermann@...e.de,
        vkoul@...nel.org, devicetree@...r.kernel.org,
        dri-devel@...ts.freedesktop.org,
        linux-arm-kernel@...ts.infradead.org, linux-fbdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-mediatek@...ts.infradead.org,
        linux-phy@...ts.infradead.org, markyacoub@...gle.com,
        Markus Schneider-Pargmann <msp@...libre.com>,
        kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v9 18/22] drm/mediatek: Add mt8195 Embedded DisplayPort
 driver

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?

> >> +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.

> >> +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?

> >> +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

Powered by blists - more mailing lists