[PATCH v17 4/8] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

[Sandor Yu]

>
> On Mon, Sep 30, 2024 at 10:18:06AM +0200, Maxime Ripard wrote:
> > On Sun, Sep 29, 2024 at 02:34:36AM GMT, Sandor Yu wrote:
> > > > > +static void cdns_hdmi_sink_config(struct cdns_mhdp8501_device
> > > > > +*mhdp) {
> > > > > +       struct drm_display_info *display =
> &mhdp->curr_conn->display_info;
> > > > > +       struct drm_connector_state *conn_state =
> > > > > +mhdp->curr_conn->state;
> > > >
> > > > That looks a bit hackish to me. We should probably provide a
> > > > helper to get the connector state the bridge is attached to.
> > >
> > > How about code change to followed, is it more clear?
> > > 370         struct drm_connector *connector = mhdp->curr_conn;
> > > 371         struct drm_connector_state *conn_state =
> connector->state;
> > > 372         struct drm_display_info *display =
> &connector->display_info;
> > > 373         struct drm_scdc *scdc = &display->hdmi.scdc;
> >
> > What I meant was that I wish bridges had a way to get their connector
> > pointer. It doesn't look like it's possible with drm_bridge_connector,
> > and we don't have access to drm_display_info anymore.
> >
> > I don't really see a good way to do this yet, so maybe that kind of
> > workaround is ok. Eventually, I guess we'll have the scrambler setup
> > in the HDMI connector helpers anyway.
> >
> > Dmitry, any idea?
>
> Unfortunately nothing significant, I didn't have time to look at the scrambler
> yet. The platforms that I'm working with either do not support HDMI 2.0 or
> require no additional setup there.
>
> Regarding the drm_connector_state, I had to go via the bridge->encoder, then
> drm_atomic_get_new_connector_for_encoder() and finally
> drm_atomic_get_new_connector_state(). I don't like the idea of storing the
> connector in drm_bridge driver data, it seems like a bad idea to me.
>

Some DRM drivers save the connector and its state after calling bridge_atomic_enable.
They then reference connector state in other parts of the driver.
Could there be any problems with this approach, assuming all subsequent uses happen after the bridge_atomic_enable call?

For the drm_connector_state in cdns_hdmi_sink_config function,
it could be removed and replace by tmds_char_rate, the parameter could be passed from bridge_atomic_enable.

For scrambler setup,
I'm thinking of proposing this function as a new SCDC API.
If it's okay, I'll create a patch and send it out for review.
We can use this API to set scrambling and clock_ratio in both my current and future drivers.
290 bool
291 drm_scdc_configure_hdmi_sink_tmds(struct drm_connector *connector,
292                                   unsigned long long tmds_char_rate,
293                                   bool scrambling_low_rates)
294 {
295         struct drm_display_info *info = &connector->display_info;
296         struct drm_scdc *scdc = &info->hdmi.scdc;
297         bool hdmi_scrambling = false;
298         bool hdmi_high_tmds_clock_ratio = false;
299
300         /* check HDMI sink type, HDMI2.0+ or not */
301         if (!info->is_hdmi || !scdc->supported)
302                 return false;
303
304         if (tmds_char_rate > HDMI_14_MAX_TMDS_CLK) {
305                 /*
306                  * TMDS Character Rate above 340MHz should
307                  * enable scrambling and TMDS_Bit_Clock_Ratio
308                  */
309                 hdmi_scrambling = true;
310                 hdmi_high_tmds_clock_ratio = true;
311         } else if (scrambling_low_rates && scdc->scrambling.low_rates) {
312                 /*
313                  * Enable scrambling when its capability be indicated
314                  * in the HF-VSDB LTE_340Mcsc_scramble bit
315                  */
316                 hdmi_scrambling = true;
317         }
318
319         /* Set TMDS bit clock ratio to 1/40 or 1/10, and enable/disable scrambling */
320         return  drm_scdc_set_scrambling(connector, hdmi_scrambling) &&
321                 drm_scdc_set_high_tmds_clock_ratio(connector, hdmi_high_tmds_clock_ratio);
322
323 }
324 EXPORT_SYMBOL(drm_scdc_configure_hdmi_sink_tmds);

> >
> > > > > +static enum drm_mode_status
> > > > > +cdns_hdmi_tmds_char_rate_valid(const struct drm_bridge *bridge,
> > > > > +                              const struct drm_display_mode
> *mode,
> > > > > +                              unsigned long long tmds_rate) {
> > > > > +       struct cdns_mhdp8501_device *mhdp =
> bridge->driver_private;
> > > > > +       union phy_configure_opts phy_cfg;
> > > > > +       int ret;
> > > > > +
> > > > > +       phy_cfg.hdmi.tmds_char_rate = tmds_rate;
> > > > > +
> > > > > +       ret = phy_validate(mhdp->phy, PHY_MODE_HDMI, 0,
> &phy_cfg);
> > > > > +       if (ret < 0)
> > > > > +               return MODE_CLOCK_RANGE;
> > > > > +
> > > > > +       return MODE_OK;
> > > > > +}
> > > > > +
> > > > > +static enum drm_mode_status
> > > > > +cdns_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
> > > > > +                           const struct drm_display_info *info,
> > > > > +                           const struct drm_display_mode
> *mode) {
> > > > > +       unsigned long long tmds_rate;
> > > > > +
> > > > > +       /* We don't support double-clocked and Interlaced modes
> */
> > > > > +       if (mode->flags & DRM_MODE_FLAG_DBLCLK ||
> > > > > +           mode->flags & DRM_MODE_FLAG_INTERLACE)
> > > > > +               return MODE_BAD;
> > > > > +
> > > > > +       if (mode->hdisplay > 3840)
> > > > > +               return MODE_BAD_HVALUE;
> > > > > +
> > > > > +       if (mode->vdisplay > 2160)
> > > > > +               return MODE_BAD_VVALUE;
> > > > > +
> > > > > +       tmds_rate = mode->clock * 1000ULL;
> > > > > +       return cdns_hdmi_tmds_char_rate_valid(bridge, mode,
> > > > > + tmds_rate); }
> > > >
> > > > Didn't we agree on creating a mode_valid helper?
> > >
> > > In fact, now I'm no idea where should add the mode_valid helper function.
> > >
> > > In struct drm_bridge_funcs, it had mode_valid() and
> hdmi_tmds_char_rate_valid().
> > >
> > > If create a new mode_valid helper function in struct
> > > drm_connector_hdmi_funcs, Is it appropriate to call another API
> > > function(tmds_char_rate_valid) at the same level within this API function?
> >
> > I'm not quite sure what you mean, but a reasonable approach to me
> > would be to turn drm_hdmi_state_helper.c hdmi_clock_valid into a
> > public function, and then call it from drm_bridge_connector mode_valid
> hook.
> >
> > It's a similar discussion to the previous one really: in order to
> > implement it properly, we need access to drm_display_info.
>
> I've sent a proposal, [1]. I don't think we should be using
> hdmi_clock_valid() directly (at least not before sorting out the EDID / hotplug
> handling in the HDMI Connector code) exactly because of the
> info->max_tmds_clock. If it gets stale, we might filter modes
> incorrectly. Also, I'm not sure if it should be left at 0 by default (or in
> drm_parse_hdmi_forum_scds()).
>
I ran into the same problem about info->max_tmds_clock when I tried to use hdmi_clock_valid to check mode_valid.
You create a function drm_hdmi_connector_mode_valid in patch[1] that does exactly what I need.

Thanks,
Sandor

> [1]
> https://lore.ke/
> rnel.org%2Fdri-devel%2F20241018-hdmi-mode-valid-v1-0-6e49ae4801f7%40
> linaro.org%2F&data=05%7C02%7Csandor.yu%40nxp.com%7Cb6594cde65474
> 3f8f1fe08dcf04effb5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> C638649469314303142%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C
> &sdata=t3B356GQ5b7NYN1tspIuTmMLf3fQZCycbJ8UrSswnEk%3D&reserved
> =0
>
> --
> With best wishes
> Dmitry