Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
Hi, On 04/09/2020 05:29, Laurent Pinchart wrote: >> Laurent mentioned that atomic_check should not change state. Note that >> cdns_mhdp_validate_mode_params also changes state, as it calculates tu_size, >> vs and line_thresh. > > .atomic_check() isn't allowed to change any global state, which means > both hardware state and data in cdns_mhdp_device. The drm_bridge_state > (and thus the cdns_mhdp_bridge_state) can be modified as it stores the > state for the atomic commit being checked. > >> There seems to be issues with mode changes, but I think the first step would >> be to clarify the >> related code a bit. cdns_mhdp_validate_mode_params() is misnamed, I think it >> should be renamed to >> calculate_tu or something like that. >> >> cdns_mhdp_bandwidth_ok() should take display_fmt or bpp as a parameter, as >> currently it digs that up >> from the current state. >> >> Probably cdns_mhdp_validate_mode_params() would be better if it doesn't >> write the result to the >> state, but returns the values. That way it could also be used to verify if >> suitable settings can be >> found, without changing the state. > > This use case is actually a very good example of proper usage of the > atomic state :-) .atomic_check() has to perform computations to verify > the atomic commit, and storing the results in the commit's state > prevents duplicating the same calculation at .atomic_commit() time. Yes, you're right. But it's still not good, as cdns_mhdp_validate_mode_params uses link details to do the calculations, but we do link training only later and thus the calculations are invalid. cdns_mhdp_validate_mode_params is also called from the HPD interrupt, and there it changes the current bridge state. link_mutex is being held in every place where cdns_mhdp_validate_mode_params is called, so I guess it's fine. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
Hi Tomi, >-Original Message- >From: Tomi Valkeinen >Sent: Thursday, September 3, 2020 12:54 PM >To: Milind Parab ; Swapnil Kashinath Jakhade >; airl...@linux.ie; dan...@ffwll.ch; >laurent.pinch...@ideasonboard.com; robh...@kernel.org; >a.ha...@samsung.com; narmstr...@baylibre.com; jo...@kwiboo.se; >jernej.skra...@siol.net; dri-devel@lists.freedesktop.org; >devicet...@vger.kernel.org; linux-ker...@vger.kernel.org >Cc: Yuti Suresh Amonkar ; jsa...@ti.com; >nsek...@ti.com; prane...@ti.com; nikhil...@ti.com >Subject: Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 >DPI/DP bridge > >EXTERNAL MAIL > > >Hi Milind, > >On 03/09/2020 09:22, Milind Parab wrote: > >> Also, note that CDNS MHDP implements DP_FRAMER_TU_p where bits 5:0 >is tu_valid_symbols. So max programmable value is 63. >> Register document gives following explanation "Number of valid symbols >> per Transfer Unit (TU). Rounded down to lower integer value (Allowed >values are 1 to (TU_size-1)" >> >> So, it says in case vs calculates to 64 (where Avail BW and Req BW are >> same) we program tu_valid_symbols = 63 > >Hmm, so "Rounded down to lower integer value" means > >floor(x) - 1 ? > >If that's the case, we need to subtract 1 in all cases, not only when req bw == >avail bw. > Explicit subtraction by 1 is not mentioned anywhere There is a hint of sub-optimal performance when vs equals 64. However need to confirm from simulations. >> Third, is about the line_threshold calculation Unlike TU_SIZE and >> Valid_Symbols, line_threshold is implementation dependent >> >> CDNS MHDP register specs gives the definition as " Video FIFO latency >threshold" >> Bits 5:0, Name "cfg_active_line_tresh", Description "Video Fifo Latency >threshold. Defines the number of FIFO rows before reading starts. This setting >depends on the transmitted video format and link rate." >> >> This parameter is the Threshold of the FIFO. For optimal performance >(considering equal write and read clock) we normally put the threshold in the >mid of the FIFO. >> Hence the reset value is fixed as 32. >> Since symbol FIFO is accessed by Pxl clock and Symbol Link Clock the >> Threshold is set to a value which is dependent on the ratio of these >> clocks >> >> line_threshold = full_fifo - fifo_ratio_due_to_clock_diff + 2 where, >> full_fifo = (vs+1) * (8/bpp) fifo_ratio_due_to_clock_diff = ((vs+1) * >> pxlclock/mhdp->link.rate - 1) / mhdp->link.num_lanes >> >> Note that line_threshold can take a max value of 63 > >That doesn't result in anything sensible. 8/bpp is always 0. Consider above statements as pseudocode. For integer division we need scale up like it was scaled by 5 bits in the original code And here 8/bpp = 1 > > Tomi > >-- >Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. >Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
Hi Tomi,
On Tue, Sep 01, 2020 at 10:46:03AM +0300, Tomi Valkeinen wrote:
> Hi Swapnil,
>
> On 31/08/2020 11:23, Swapnil Jakhade wrote:
>
> > +static int cdns_mhdp_validate_mode_params(struct cdns_mhdp_device *mhdp,
> > + const struct drm_display_mode *mode,
> > + struct drm_bridge_state *bridge_state)
> > +{
> > + u32 tu_size = 30, line_thresh1, line_thresh2, line_thresh = 0;
> > + u32 rate, vs, vs_f, required_bandwidth, available_bandwidth;
> > + struct cdns_mhdp_bridge_state *state;
> > + int pxlclock;
> > + u32 bpp;
> > +
> > + state = to_cdns_mhdp_bridge_state(bridge_state);
> > +
> > + pxlclock = mode->crtc_clock;
> > +
> > + /* Get rate in MSymbols per second per lane */
> > + rate = mhdp->link.rate / 1000;
> > +
> > + bpp = cdns_mhdp_get_bpp(>display_fmt);
>
> None of the above are used when calling cdns_mhdp_bandwidth_ok(). For
> clarity, I'd move the above
> lines a bit closer to where they are needed, as currently it makes me think
> the above values are
> used when checking the bandwidth.
>
> > + if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes,
> > + mhdp->link.rate)) {
> > + dev_err(mhdp->dev, "%s: Not enough BW for %s (%u lanes at %u
> > Mbps)\n",
> > + __func__, mode->name, mhdp->link.num_lanes,
> > + mhdp->link.rate / 100);
> > + return -EINVAL;
> > + }
> > +
> > + /* find optimal tu_size */
> > + required_bandwidth = pxlclock * bpp / 8;
> > + available_bandwidth = mhdp->link.num_lanes * rate;
> > + do {
> > + tu_size += 2;
> > +
> > + vs_f = tu_size * required_bandwidth / available_bandwidth;
> > + vs = vs_f / 1000;
> > + vs_f = vs_f % 1000;
> > + /* Downspreading is unused currently */
> > + } while ((vs == 1 || ((vs_f > 850 || vs_f < 100) && vs_f != 0) ||
> > +tu_size - vs < 2) && tu_size < 64);
> > +
> > + if (vs > 64) {
> > + dev_err(mhdp->dev,
> > + "%s: No space for framing %s (%u lanes at %u Mbps)\n",
> > + __func__, mode->name, mhdp->link.num_lanes,
> > + mhdp->link.rate / 100);
> > + return -EINVAL;
> > + }
> > +
> > + if (vs == tu_size)
> > + vs = tu_size - 1;
> > +
> > + line_thresh1 = ((vs + 1) << 5) * 8 / bpp;
> > + line_thresh2 = (pxlclock << 5) / 1000 / rate * (vs + 1) - (1 << 5);
> > + line_thresh = line_thresh1 - line_thresh2 / mhdp->link.num_lanes;
> > + line_thresh = (line_thresh >> 5) + 2;
> > +
> > + state->vs = vs;
> > + state->tu_size = tu_size;
> > + state->line_thresh = line_thresh;
> > +
> > + return 0;
> > +}
> > +
> > +static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
> > + struct drm_bridge_state *bridge_state,
> > + struct drm_crtc_state *crtc_state,
> > + struct drm_connector_state *conn_state)
> > +{
> > + struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
> > + const struct drm_display_mode *mode = _state->adjusted_mode;
> > + int ret;
> > +
> > + mutex_lock(>link_mutex);
> > +
> > + if (!mhdp->plugged) {
> > + mhdp->link.rate = mhdp->host.link_rate;
> > + mhdp->link.num_lanes = mhdp->host.lanes_cnt;
> > + }
> > +
> > + ret = cdns_mhdp_validate_mode_params(mhdp, mode, bridge_state);
> > +
> > + mutex_unlock(>link_mutex);
> > +
> > + return ret;
> > +}
>
> Laurent mentioned that atomic_check should not change state. Note that
> cdns_mhdp_validate_mode_params also changes state, as it calculates tu_size,
> vs and line_thresh.
.atomic_check() isn't allowed to change any global state, which means
both hardware state and data in cdns_mhdp_device. The drm_bridge_state
(and thus the cdns_mhdp_bridge_state) can be modified as it stores the
state for the atomic commit being checked.
> There seems to be issues with mode changes, but I think the first step would
> be to clarify the
> related code a bit. cdns_mhdp_validate_mode_params() is misnamed, I think it
> should be renamed to
> calculate_tu or something like that.
>
> cdns_mhdp_bandwidth_ok() should take display_fmt or bpp as a parameter, as
> currently it digs that up
> from the current state.
>
> Probably cdns_mhdp_validate_mode_params() would be better if it doesn't write
> the result to the
> state, but returns the values. That way it could also be used to verify if
> suitable settings can be
> found, without changing the state.
This use case is actually a very good example of proper usage of the
atomic state :-) .atomic_check() has to perform computations to verify
the atomic commit, and storing the results in the commit's state
prevents duplicating the same calculation at .atomic_commit() time.
> The are two issues I see with some testing, which are probably related.
>
> The first
Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
Hi Milind, On 03/09/2020 09:22, Milind Parab wrote: > Also, note that CDNS MHDP implements DP_FRAMER_TU_p where bits 5:0 is > tu_valid_symbols. So max programmable value is 63. > Register document gives following explanation > "Number of valid symbols per Transfer Unit (TU). Rounded down to lower > integer value (Allowed values are 1 to (TU_size-1)" > > So, it says in case vs calculates to 64 (where Avail BW and Req BW are same) > we program tu_valid_symbols = 63 Hmm, so "Rounded down to lower integer value" means floor(x) - 1 ? If that's the case, we need to subtract 1 in all cases, not only when req bw == avail bw. > Third, is about the line_threshold calculation > Unlike TU_SIZE and Valid_Symbols, line_threshold is implementation dependent > > CDNS MHDP register specs gives the definition as " Video FIFO latency > threshold" > Bits 5:0, Name "cfg_active_line_tresh", Description "Video Fifo Latency > threshold. Defines the number of FIFO rows before reading starts. This > setting depends on the transmitted video format and link rate." > > This parameter is the Threshold of the FIFO. For optimal performance > (considering equal write and read clock) we normally put the threshold in the > mid of the FIFO. > Hence the reset value is fixed as 32. > Since symbol FIFO is accessed by Pxl clock and Symbol Link Clock the > Threshold is set to a value which is dependent on the ratio of these clocks > > line_threshold = full_fifo - fifo_ratio_due_to_clock_diff + 2 > where, > full_fifo = (vs+1) * (8/bpp) > fifo_ratio_due_to_clock_diff = ((vs+1) * pxlclock/mhdp->link.rate - 1) / > mhdp->link.num_lanes > > Note that line_threshold can take a max value of 63 That doesn't result in anything sensible. 8/bpp is always 0. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
Hi Tomi, >-Original Message- >From: Tomi Valkeinen >Sent: Tuesday, September 1, 2020 2:05 PM >To: Swapnil Kashinath Jakhade ; airl...@linux.ie; >dan...@ffwll.ch; laurent.pinch...@ideasonboard.com; robh...@kernel.org; >a.ha...@samsung.com; narmstr...@baylibre.com; jo...@kwiboo.se; >jernej.skra...@siol.net; dri-devel@lists.freedesktop.org; >devicet...@vger.kernel.org; linux-ker...@vger.kernel.org >Cc: Milind Parab ; Yuti Suresh Amonkar >; jsa...@ti.com; nsek...@ti.com; >prane...@ti.com; nikhil...@ti.com >Subject: Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 >DPI/DP bridge > >EXTERNAL MAIL > > >Hi Swapnil, > >On 31/08/2020 11:23, Swapnil Jakhade wrote: > >> +line_thresh1 = ((vs + 1) << 5) * 8 / bpp; >> +line_thresh2 = (pxlclock << 5) / 1000 / rate * (vs + 1) - (1 << 5); >> +line_thresh = line_thresh1 - line_thresh2 / mhdp->link.num_lanes; >> +line_thresh = (line_thresh >> 5) + 2; > >These calculations do not seem to go correctly. There's no comment what's >the logic here, but e.g. >for 640x480 (pxlclock 31500) with 1.62Gbps link, I get vs=4, and then the >second line above comes to: > >(31500 << 5) / 1000 / 162 * (4+1) - (1<<5) = -0.8893 > >The result is line_thresh of 100663299. > Yes, there is a mistake in the above equations. I will detailed it here Before that there are other related issues which also needs clarification First is about the TU_SIZE. The DP spec says, in SST mode the TU_SIZE can take any even value from 32 to 64. The advantages of having a smaller TU size is not explicitly mentioned, however logically it helps stream down-spreading and would be beneficial in low bandwidth low buffer applications. Hence, we can consider that selecting a lower TU size is an optimization we can consider implementing later. For now let us fix TU_SIZE = 64 Second, is Valid Symbol Length "vs" For a fixed TU valid symbol length will depend in the Ratio or Pixel clock and Link symbol Rate (lanes * rate) vs = 64 * required_bandwidth / available_bandwidth Where, required_bandwidth = pxlclock * (bpp/8) available_bandwidth = mhdp->link.num_lanes * mhdp->link.rate Also, note that CDNS MHDP implements DP_FRAMER_TU_p where bits 5:0 is tu_valid_symbols. So max programmable value is 63. Register document gives following explanation "Number of valid symbols per Transfer Unit (TU). Rounded down to lower integer value (Allowed values are 1 to (TU_size-1)" So, it says in case vs calculates to 64 (where Avail BW and Req BW are same) we program tu_valid_symbols = 63 Third, is about the line_threshold calculation Unlike TU_SIZE and Valid_Symbols, line_threshold is implementation dependent CDNS MHDP register specs gives the definition as " Video FIFO latency threshold" Bits 5:0, Name "cfg_active_line_tresh", Description "Video Fifo Latency threshold. Defines the number of FIFO rows before reading starts. This setting depends on the transmitted video format and link rate." This parameter is the Threshold of the FIFO. For optimal performance (considering equal write and read clock) we normally put the threshold in the mid of the FIFO. Hence the reset value is fixed as 32. Since symbol FIFO is accessed by Pxl clock and Symbol Link Clock the Threshold is set to a value which is dependent on the ratio of these clocks line_threshold = full_fifo - fifo_ratio_due_to_clock_diff + 2 where, full_fifo = (vs+1) * (8/bpp) fifo_ratio_due_to_clock_diff = ((vs+1) * pxlclock/mhdp->link.rate - 1) / mhdp->link.num_lanes Note that line_threshold can take a max value of 63 > Tomi > >-- >Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. >Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
Hi Swapnil, On 31/08/2020 11:23, Swapnil Jakhade wrote: > + line_thresh1 = ((vs + 1) << 5) * 8 / bpp; > + line_thresh2 = (pxlclock << 5) / 1000 / rate * (vs + 1) - (1 << 5); > + line_thresh = line_thresh1 - line_thresh2 / mhdp->link.num_lanes; > + line_thresh = (line_thresh >> 5) + 2; These calculations do not seem to go correctly. There's no comment what's the logic here, but e.g. for 640x480 (pxlclock 31500) with 1.62Gbps link, I get vs=4, and then the second line above comes to: (31500 << 5) / 1000 / 162 * (4+1) - (1<<5) = -0.8893 The result is line_thresh of 100663299. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
On 01/09/2020 10:46, Tomi Valkeinen wrote: > I think the above suggests that the driver is not properly updating all the > registers based on the > new mode and link. I tried adding cdns_mhdp_validate_mode_params() call to > cdns_mhdp_atomic_enable(), so that tu-size etc will be calculated, but that > did not fix the problem. Oh, it actually did fix the problem. It was just that my first hack updated the old state, but after changing the code to call cdns_mhdp_atomic_enable() with new_state I see it helps with the issue. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
Hi Swapnil,
On 31/08/2020 11:23, Swapnil Jakhade wrote:
> +static int cdns_mhdp_validate_mode_params(struct cdns_mhdp_device *mhdp,
> + const struct drm_display_mode *mode,
> + struct drm_bridge_state *bridge_state)
> +{
> + u32 tu_size = 30, line_thresh1, line_thresh2, line_thresh = 0;
> + u32 rate, vs, vs_f, required_bandwidth, available_bandwidth;
> + struct cdns_mhdp_bridge_state *state;
> + int pxlclock;
> + u32 bpp;
> +
> + state = to_cdns_mhdp_bridge_state(bridge_state);
> +
> + pxlclock = mode->crtc_clock;
> +
> + /* Get rate in MSymbols per second per lane */
> + rate = mhdp->link.rate / 1000;
> +
> + bpp = cdns_mhdp_get_bpp(>display_fmt);
None of the above are used when calling cdns_mhdp_bandwidth_ok(). For clarity,
I'd move the above
lines a bit closer to where they are needed, as currently it makes me think the
above values are
used when checking the bandwidth.
> + if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes,
> + mhdp->link.rate)) {
> + dev_err(mhdp->dev, "%s: Not enough BW for %s (%u lanes at %u
> Mbps)\n",
> + __func__, mode->name, mhdp->link.num_lanes,
> + mhdp->link.rate / 100);
> + return -EINVAL;
> + }
> +
> + /* find optimal tu_size */
> + required_bandwidth = pxlclock * bpp / 8;
> + available_bandwidth = mhdp->link.num_lanes * rate;
> + do {
> + tu_size += 2;
> +
> + vs_f = tu_size * required_bandwidth / available_bandwidth;
> + vs = vs_f / 1000;
> + vs_f = vs_f % 1000;
> + /* Downspreading is unused currently */
> + } while ((vs == 1 || ((vs_f > 850 || vs_f < 100) && vs_f != 0) ||
> + tu_size - vs < 2) && tu_size < 64);
> +
> + if (vs > 64) {
> + dev_err(mhdp->dev,
> + "%s: No space for framing %s (%u lanes at %u Mbps)\n",
> + __func__, mode->name, mhdp->link.num_lanes,
> + mhdp->link.rate / 100);
> + return -EINVAL;
> + }
> +
> + if (vs == tu_size)
> + vs = tu_size - 1;
> +
> + line_thresh1 = ((vs + 1) << 5) * 8 / bpp;
> + line_thresh2 = (pxlclock << 5) / 1000 / rate * (vs + 1) - (1 << 5);
> + line_thresh = line_thresh1 - line_thresh2 / mhdp->link.num_lanes;
> + line_thresh = (line_thresh >> 5) + 2;
> +
> + state->vs = vs;
> + state->tu_size = tu_size;
> + state->line_thresh = line_thresh;
> +
> + return 0;
> +}
> +
> +static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> +{
> + struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
> + const struct drm_display_mode *mode = _state->adjusted_mode;
> + int ret;
> +
> + mutex_lock(>link_mutex);
> +
> + if (!mhdp->plugged) {
> + mhdp->link.rate = mhdp->host.link_rate;
> + mhdp->link.num_lanes = mhdp->host.lanes_cnt;
> + }
> +
> + ret = cdns_mhdp_validate_mode_params(mhdp, mode, bridge_state);
> +
> + mutex_unlock(>link_mutex);
> +
> + return ret;
> +}
Laurent mentioned that atomic_check should not change state. Note that
cdns_mhdp_validate_mode_params also changes state, as it calculates tu_size, vs
and line_thresh.
There seems to be issues with mode changes, but I think the first step would be
to clarify the
related code a bit. cdns_mhdp_validate_mode_params() is misnamed, I think it
should be renamed to
calculate_tu or something like that.
cdns_mhdp_bandwidth_ok() should take display_fmt or bpp as a parameter, as
currently it digs that up
from the current state.
Probably cdns_mhdp_validate_mode_params() would be better if it doesn't write
the result to the
state, but returns the values. That way it could also be used to verify if
suitable settings can be
found, without changing the state.
The are two issues I see with some testing, which are probably related.
The first one is that if I run kmstest with a new mode, I see tu-size & co
being calculated. But the
calculation happens before link training, which doesn't make sense. So I think
what's done here is
that atomic_check causes tu-size calculations, then atomic_enable does link
training and enables the
video.
The second happens when my monitor fails with the first CR after power-on, and
the driver drops
number-of-lanes to 2. It goes like this:
The driver is loaded. Based on EDID, fbdev is created with 1920x1200. Link
training is done, which
has the CR issue, and because of that the actual mode that we get is 1280x960.
I get a proper
picture here, so far so good.
Then if I run kmstest, it only allows 1280x960 as the
Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
Hi Swapnil,
Thank you for the patch.
On Mon, Aug 31, 2020 at 10:23:34AM +0200, Swapnil Jakhade wrote:
> Add a new DRM bridge driver for Cadence MHDP8546 DPTX IP used in TI J721E
> SoC. MHDP DPTX IP is the component that complies with VESA DisplayPort (DP)
> and embedded Display Port (eDP) standards. It integrates uCPU running the
> embedded Firmware (FW) interfaced over APB interface.
>
> Basically, it takes a DPI stream as input and outputs it encoded in DP
> format. Currently, it supports only SST mode.
>
> Co-developed-by: Tomi Valkeinen
> Signed-off-by: Tomi Valkeinen
> Co-developed-by: Jyri Sarha
> Signed-off-by: Jyri Sarha
> Signed-off-by: Quentin Schulz
> Signed-off-by: Yuti Amonkar
> Signed-off-by: Swapnil Jakhade
> ---
> drivers/gpu/drm/bridge/Kconfig|2 +
> drivers/gpu/drm/bridge/Makefile |1 +
> drivers/gpu/drm/bridge/cadence/Kconfig| 11 +
> drivers/gpu/drm/bridge/cadence/Makefile |3 +
> .../drm/bridge/cadence/cdns-mhdp8546-core.c | 2548 +
> .../drm/bridge/cadence/cdns-mhdp8546-core.h | 402 +++
> 6 files changed, 2967 insertions(+)
> create mode 100644 drivers/gpu/drm/bridge/cadence/Kconfig
> create mode 100644 drivers/gpu/drm/bridge/cadence/Makefile
> create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3e11af4e9f63..ef91646441b1 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -241,6 +241,8 @@ source "drivers/gpu/drm/bridge/analogix/Kconfig"
>
> source "drivers/gpu/drm/bridge/adv7511/Kconfig"
>
> +source "drivers/gpu/drm/bridge/cadence/Kconfig"
> +
> source "drivers/gpu/drm/bridge/synopsys/Kconfig"
>
> endmenu
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index c589a6a7cbe1..2b3aff104e46 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -25,4 +25,5 @@ obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
> obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi.o
>
> obj-y += analogix/
> +obj-y += cadence/
> obj-y += synopsys/
> diff --git a/drivers/gpu/drm/bridge/cadence/Kconfig
> b/drivers/gpu/drm/bridge/cadence/Kconfig
> new file mode 100644
> index ..f49d77eb7814
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config DRM_CDNS_MHDP8546
> + tristate "Cadence DPI/DP bridge"
> + select DRM_KMS_HELPER
> + select DRM_PANEL_BRIDGE
> + depends on OF
> + help
> + Support Cadence DPI to DP bridge. This is an internal
> + bridge and is meant to be directly embedded in a SoC.
> + It takes a DPI stream as input and outputs it encoded
> + in DP format.
> diff --git a/drivers/gpu/drm/bridge/cadence/Makefile
> b/drivers/gpu/drm/bridge/cadence/Makefile
> new file mode 100644
> index ..676739cdf5e6
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/cadence/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_DRM_CDNS_MHDP8546) += cdns-mhdp8546.o
> +cdns-mhdp8546-y := cdns-mhdp8546-core.o
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> new file mode 100644
> index ..14be6f370d6e
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -0,0 +1,2548 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Cadence MHDP8546 DP bridge driver.
> + *
> + * Copyright (C) 2020 Cadence Design Systems, Inc.
> + *
> + * Authors: Quentin Schulz
> + * Swapnil Jakhade
> + * Yuti Amonkar
> + * Tomi Valkeinen
> + * Jyri Sarha
> + *
> + * TODO:
> + * - Implement optimized mailbox communication using mailbox interrupts
> + * - Add support for power management
> + * - Add support for features like audio, MST and fast link training
> + * - Implement request_fw_cancel to handle HW_STATE
> + * - Fix asynchronous loading of firmware implementation
> + * - Add DRM helper function for cdns_mhdp_lower_link_rate
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include "cdns-mhdp8546-core.h"
> +
> +static int cdns_mhdp_mailbox_read(struct cdns_mhdp_device *mhdp)
> +{
> + int ret, empty;
> +
> + WARN_ON(!mutex_is_locked(>mbox_mutex));
> +
> + ret = readx_poll_timeout(readl, mhdp->regs + CDNS_MAILBOX_EMPTY,
> + empty, !empty, MAILBOX_RETRY_US,
> +
Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
Hi Swapnil,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v5.9-rc3 next-20200828]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Swapnil-Jakhade/drm-Add-support-for-Cadence-MHDP8546-DPI-DP-bridge-and-J721E-wrapper/20200831-162549
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=sparc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c: In function
'cdns_mhdp_fw_activate':
>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:749:10: warning:
>> conversion from 'long unsigned int' to 'u32' {aka 'unsigned int'} changes
>> value from '18446744073709551613' to '4294967293' [-Woverflow]
749 | writel(~CDNS_APB_INT_MASK_SW_EVENT_INT,
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c: In function
'cdns_mhdp_fill_host_caps':
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:1355:2: error: implicit
declaration of function 'phy_get_attrs' [-Werror=implicit-function-declaration]
1355 | phy_get_attrs(mhdp->phy, );
| ^
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:1361:19: error: 'struct
phy_attrs' has no member named 'max_link_rate'
1361 | link_rate = attrs.max_link_rate;
| ^
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c: In function
'cdns_mhdp_attach':
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:1680:10: warning:
conversion from 'long unsigned int' to 'u32' {aka 'unsigned int'} changes value
from '18446744073709551613' to '4294967293' [-Woverflow]
1680 | writel(~CDNS_APB_INT_MASK_SW_EVENT_INT,
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c: In function
'cdns_mhdp_bridge_hpd_enable':
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:2152:10: warning:
conversion from 'long unsigned int' to 'u32' {aka 'unsigned int'} changes value
from '18446744073709551613' to '4294967293' [-Woverflow]
2152 | writel(~CDNS_APB_INT_MASK_SW_EVENT_INT,
cc1: some warnings being treated as errors
#
https://github.com/0day-ci/linux/commit/f9a2fb8569e017b7eaba3a94afc9581179c2b62b
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Swapnil-Jakhade/drm-Add-support-for-Cadence-MHDP8546-DPI-DP-bridge-and-J721E-wrapper/20200831-162549
git checkout f9a2fb8569e017b7eaba3a94afc9581179c2b62b
vim +749 drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
692
693 static int cdns_mhdp_fw_activate(const struct firmware *fw,
694 struct cdns_mhdp_device *mhdp)
695 {
696 unsigned int reg;
697 int ret;
698
699 /* Release uCPU reset and stall it. */
700 writel(CDNS_CPU_STALL, mhdp->regs + CDNS_APB_CTRL);
701
702 memcpy_toio(mhdp->regs + CDNS_MHDP_IMEM, fw->data, fw->size);
703
704 /* Leave debug mode, release stall */
705 writel(0, mhdp->regs + CDNS_APB_CTRL);
706
707 /*
708 * Wait for the KEEP_ALIVE "message" on the first 8 bits.
709 * Updated each sched "tick" (~2ms)
710 */
711 ret = readl_poll_timeout(mhdp->regs + CDNS_KEEP_ALIVE, reg,
712 reg & CDNS_KEEP_ALIVE_MASK, 500,
713 CDNS_KEEP_ALIVE_TIMEOUT);
714 if (ret) {
715 dev_err(mhdp->dev,
716 "device didn't give any life sign: reg %d\n",
reg);
717 return ret;
718 }
719
720 ret = cdns_mhdp_check_fw_version(mhdp);
721 if (ret)
722 return ret;
723
724 /* Init events to 0 as it's not cleared by FW at boot but on
read */
725 readl(mhdp->regs + CDNS_SW_EVENT0);
726 readl(mhdp->regs + CDNS_SW_EVENT1);
727 readl(mhdp->regs + CDNS_SW_EVENT2);
728 readl(mhdp->regs + CDNS_SW_EVENT3);
729
730 /* Activate uCPU */
731 ret = cdns_mhdp_set_firmware_active(mhdp, true);
732 if (ret)
733 return ret;
734
735 spin_lock(>start_lock);
736
Re: [PATCH v9 2/3] drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge
Hi Swapnil,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v5.9-rc3 next-20200828]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Swapnil-Jakhade/drm-Add-support-for-Cadence-MHDP8546-DPI-DP-bridge-and-J721E-wrapper/20200831-162549
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
In file included from include/linux/err.h:5,
from include/linux/clk.h:12,
from drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:22:
include/linux/scatterlist.h: In function 'sg_set_buf':
arch/m68k/include/asm/page_mm.h:169:49: warning: ordered comparison of
pointer with null pointer [-Wextra]
169 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void
*)PAGE_OFFSET && (void *)(kaddr) < high_memory)
| ^~
include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
78 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
143 | BUG_ON(!virt_addr_valid(buf));
| ^~
include/linux/scatterlist.h:143:10: note: in expansion of macro
'virt_addr_valid'
143 | BUG_ON(!virt_addr_valid(buf));
| ^~~
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c: In function
'cdns_mhdp_fill_host_caps':
>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:1355:2: error: implicit
>> declaration of function 'phy_get_attrs'
>> [-Werror=implicit-function-declaration]
1355 | phy_get_attrs(mhdp->phy, );
| ^
>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:1361:19: error: 'struct
>> phy_attrs' has no member named 'max_link_rate'
1361 | link_rate = attrs.max_link_rate;
| ^
cc1: some warnings being treated as errors
#
https://github.com/0day-ci/linux/commit/f9a2fb8569e017b7eaba3a94afc9581179c2b62b
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Swapnil-Jakhade/drm-Add-support-for-Cadence-MHDP8546-DPI-DP-bridge-and-J721E-wrapper/20200831-162549
git checkout f9a2fb8569e017b7eaba3a94afc9581179c2b62b
vim +/phy_get_attrs +1355 drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
1348
1349 static void cdns_mhdp_fill_host_caps(struct cdns_mhdp_device *mhdp)
1350 {
1351 unsigned int link_rate;
1352 struct phy_attrs attrs;
1353
1354 /* Get source capabilities based on PHY attributes */
> 1355 phy_get_attrs(mhdp->phy, );
1356
1357 mhdp->host.lanes_cnt = attrs.bus_width;
1358 if (!mhdp->host.lanes_cnt)
1359 mhdp->host.lanes_cnt = 4;
1360
> 1361 link_rate = attrs.max_link_rate;
1362 if (!link_rate)
1363 link_rate = drm_dp_bw_code_to_link_rate(DP_LINK_BW_8_1);
1364 else
1365 /* PHY uses Mb/s, DRM uses tens of kb/s. */
1366 link_rate *= 100;
1367
1368 mhdp->host.link_rate = link_rate;
1369 mhdp->host.volt_swing = CDNS_VOLT_SWING(3);
1370 mhdp->host.pre_emphasis = CDNS_PRE_EMPHASIS(3);
1371 mhdp->host.pattern_supp = CDNS_SUPPORT_TPS(1) |
1372CDNS_SUPPORT_TPS(2) |
CDNS_SUPPORT_TPS(3) |
1373CDNS_SUPPORT_TPS(4);
1374 mhdp->host.lane_mapping = CDNS_LANE_MAPPING_NORMAL;
1375 mhdp->host.fast_link = false;
1376 mhdp->host.enhanced = true;
1377 mhdp->host.scrambler = true;
1378 mhdp->host.ssc = false;
1379 }
1380
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
