Re: [PATCH 1/2] drm/mediatek: Switch the hdmi bridge ops to the atomic versions

[Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>]

Hi,

On 25.03.21 11:43, Enric Balletbo i Serra wrote:
> Hi Dafna,
>
> Thank you for your patch. It'd be nice if you can cc the linux-mediatek ML for
> next version, so Mediatek people is more aware of this change. IMHO cc'ing the
> lkml is also a good practice.

ok, I added all the mails from get_maintainer.pl. linux-mediatek was not there,
I can add it.

> On 24/3/21 20:12, Dafna Hirschfeld wrote:
> > The bridge operation '.enable' and the audio cb '.get_eld'
> > access hdmi->conn. In the future we will want to support
> > the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR and then we will
> > not have direct access to the connector.
> > The atomic version '.atomic_enable' allows accessing the
> > current connector from the state.
> > This patch switches the bridge to the atomic version and
> > saves the current connector in a new field 'curr_conn'.
> >
> > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/mediatek/mtk_hdmi.c | 41 ++++++++++++++++++++---------
> >   1 file changed, 29 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> > index 8ee55f9e2954..9f415c508b33 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> > @@ -154,6 +154,7 @@ struct mtk_hdmi {
> >   	struct drm_bridge bridge;
> >   	struct drm_bridge *next_bridge;
> >   	struct drm_connector conn;
> > +	struct drm_connector *curr_conn;
>
> Adding a new drm_connector (curr_conn) is something that surprised me at the
> beginning, then I saw that in the second patch you remove the first
> drm_connector (conn). I didn't test it yet, but did you test this patch alone?
> without the second one, maybe I'm missing something but looks to me that this
> patch alone breaks more functionalities of the driver? (and I know that the
> driver is broken right now)

Hi, I didn't test it alone, but indeed this patch was wirrten with the intention
that 'conn' will be removed next patch. But I don't think that patch should break
functionality.

> Is really needed to create a new drm_connector to switch to the atomic versions?

No, indeed this is the wrong patch to add curr_conn, I should add curr_conn to the
second patch or maybe to a seperate last patch so not to overload the patch.

Thanks,
Dafna

> >   	struct device *dev;
> >   	const struct mtk_hdmi_conf *conf;
> >   	struct phy *phy;
> > @@ -974,7 +975,7 @@ static int mtk_hdmi_setup_avi_infoframe(struct mtk_hdmi *hdmi,
> >   	ssize_t err;
> >   
> >   	err = drm_hdmi_avi_infoframe_from_display_mode(&frame,
> > -						       &hdmi->conn, mode);
> > +						       hdmi->curr_conn, mode);
> >   	if (err < 0) {
> >   		dev_err(hdmi->dev,
> >   			"Failed to get AVI infoframe from mode: %zd\n", err);
> > @@ -1054,7 +1055,7 @@ static int mtk_hdmi_setup_vendor_specific_infoframe(struct mtk_hdmi *hdmi,
> >   	ssize_t err;
> >   
> >   	err = drm_hdmi_vendor_infoframe_from_display_mode(&frame,
> > -							  &hdmi->conn, mode);
> > +							  hdmi->curr_conn, mode);
> >   	if (err) {
> >   		dev_err(hdmi->dev,
> >   			"Failed to get vendor infoframe from mode: %zd\n", err);
> > @@ -1357,7 +1358,8 @@ static bool mtk_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
> >   	return true;
> >   }
> >   
> > -static void mtk_hdmi_bridge_disable(struct drm_bridge *bridge)
> > +static void mtk_hdmi_bridge_atomic_disable(struct drm_bridge *bridge,
> > +					   struct drm_bridge_state *old_bridge_state)
> >   {
> >   	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
> >   
> > @@ -1368,10 +1370,13 @@ static void mtk_hdmi_bridge_disable(struct drm_bridge *bridge)
> >   	clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_HDMI_PIXEL]);
> >   	clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_HDMI_PLL]);
> >   
> > +	hdmi->curr_conn = NULL;
> > +
> >   	hdmi->enabled = false;
> >   }
> >   
> > -static void mtk_hdmi_bridge_post_disable(struct drm_bridge *bridge)
> > +static void mtk_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
> > +						struct drm_bridge_state *old_state)
> >   {
> >   	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
> >   
> > @@ -1406,7 +1411,8 @@ static void mtk_hdmi_bridge_mode_set(struct drm_bridge *bridge,
> >   	drm_mode_copy(&hdmi->mode, adjusted_mode);
> >   }
> >   
> > -static void mtk_hdmi_bridge_pre_enable(struct drm_bridge *bridge)
> > +static void mtk_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> > +					      struct drm_bridge_state *old_state)
> >   {
> >   	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
> >   
> > @@ -1426,10 +1432,16 @@ static void mtk_hdmi_send_infoframe(struct mtk_hdmi *hdmi,
> >   		mtk_hdmi_setup_vendor_specific_infoframe(hdmi, mode);
> >   }
> >   
> > -static void mtk_hdmi_bridge_enable(struct drm_bridge *bridge)
> > +static void mtk_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
> > +					  struct drm_bridge_state *old_state)
> >   {
> > +	struct drm_atomic_state *state = old_state->base.state;
> >   	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
> >   
> > +	/* Retrieve the connector through the atomic state. */
> > +	hdmi->curr_conn = drm_atomic_get_new_connector_for_encoder(state,
> > +								   bridge->encoder);
> > +
> >   	mtk_hdmi_output_set_display_mode(hdmi, &hdmi->mode);
> >   	clk_prepare_enable(hdmi->clk[MTK_HDMI_CLK_HDMI_PLL]);
> >   	clk_prepare_enable(hdmi->clk[MTK_HDMI_CLK_HDMI_PIXEL]);
> > @@ -1440,13 +1452,16 @@ static void mtk_hdmi_bridge_enable(struct drm_bridge *bridge)
> >   }
> >   
> >   static const struct drm_bridge_funcs mtk_hdmi_bridge_funcs = {
> > +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> > +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> > +	.atomic_reset = drm_atomic_helper_bridge_reset,
> >   	.attach = mtk_hdmi_bridge_attach,
> >   	.mode_fixup = mtk_hdmi_bridge_mode_fixup,
> > -	.disable = mtk_hdmi_bridge_disable,
> > -	.post_disable = mtk_hdmi_bridge_post_disable,
> > +	.atomic_disable = mtk_hdmi_bridge_atomic_disable,
> > +	.atomic_post_disable = mtk_hdmi_bridge_atomic_post_disable,
> >   	.mode_set = mtk_hdmi_bridge_mode_set,
> > -	.pre_enable = mtk_hdmi_bridge_pre_enable,
> > -	.enable = mtk_hdmi_bridge_enable,
> > +	.atomic_pre_enable = mtk_hdmi_bridge_atomic_pre_enable,
> > +	.atomic_enable = mtk_hdmi_bridge_atomic_enable,
> >   };
> >   
> >   static int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi *hdmi,
> > @@ -1662,8 +1677,10 @@ static int mtk_hdmi_audio_get_eld(struct device *dev, void *data, uint8_t *buf,
> >   {
> >   	struct mtk_hdmi *hdmi = dev_get_drvdata(dev);
> >   
> > -	memcpy(buf, hdmi->conn.eld, min(sizeof(hdmi->conn.eld), len));
> > -
> > +	if (hdmi->curr_conn)
> > +		memcpy(buf, hdmi->curr_conn->eld, min(sizeof(hdmi->curr_conn->eld), len));
> > +	else
> > +		memset(buf, 0, len);
> >   	return 0;
> >   }
> >   
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel