| Message ID | 20210511042043.592802-1-bjorn.andersson@linaro.org |
|---|---|
| Headers | show |
| Series | drm/msm/dp: Add support for SC8180x eDP controller | expand |
Hi Bjorn I had a quick glance on the series and before getting to other things wanted to know how you are initializing two different connectors for DP & EDP resp. The connector type for DP should be DRM_MODE_CONNECTOR_DisplayPort and eDP should be DRM_MODE_CONNECTOR_eDP. We need both to be created so that both EDP and DP can be supported concurrently. Will these changes work for concurrent eDP and DP case? Thanks Abhinav On 2021-05-10 21:20, Bjorn Andersson wrote: > The first patch in the series is somewhat unrelated to the support, but > simplifies reasoning and debugging of timing related issues. > > The second patch introduces support for dealing with different register > block > layouts, which is used in the forth patch to describe the hardware > blocks found > in the SC8180x eDP block. > > The third patch configures the INTF_CONFIG register, which carries the > configuration for widebus handling. As with the DPU the bootloader > enables > widebus and we need to disable it, or implement support for adjusting > the > timing. > > Bjorn Andersson (4): > drm/msm/dp: Simplify the mvid/nvid calculation > drm/msm/dp: Store each subblock in the io region > drm/msm/dp: Initialize the INTF_CONFIG register > drm/msm/dp: Add support for SC8180x eDP > > drivers/gpu/drm/msm/dp/dp_catalog.c | 99 +++++++---------------------- > drivers/gpu/drm/msm/dp/dp_display.c | 1 + > drivers/gpu/drm/msm/dp/dp_parser.c | 22 +++++++ > drivers/gpu/drm/msm/dp/dp_parser.h | 8 +++ > 4 files changed, 53 insertions(+), 77 deletions(-)
On Tue 18 May 22:41 CDT 2021, abhinavk@codeaurora.org wrote: > Hi Bjorn > > I had a quick glance on the series and before getting to other things wanted > to know how you are initializing two different connectors for > DP & EDP resp. > > The connector type for DP should be DRM_MODE_CONNECTOR_DisplayPort and eDP > should be DRM_MODE_CONNECTOR_eDP. As far as I've been able to conclude there is no eDP support in the upstream DPU driver; an encoder of type DRM_MODE_ENCODER_TMDS will only attach to INTF_DP. > We need both to be created so that both EDP and DP can be supported > concurrently. > Further more the DP controller driver has a global variable to track state and the INTF-picker will always pick the interface of index 0 when setting up the DP controller. > Will these changes work for concurrent eDP and DP case? > The proposed changes are all that I need to get eDP working on my sc8180x laptop. But the DPU code does not currently support more than a single DP interface - and that has to be on the first INTF_DP that the DPU driver knows about. But this is a limitation we should fix, rather than claiming that you can only have one of each. Further more, afaict the sc7280 DP controller can do both DP and eDP, so it would make sense not to distinguish the interfaces as eDP or DP - just because the product in mind will use eDP. PS. I've currently disabled the eDP interface on my laptop and am working on trying to get Type-C DP working. Once that's in place I'd need a better INTF/encoder picker - because the current model of just picking INTF_DP 0 (or in a sequential fashion) won't work. Regards, Bjorn > Thanks > > Abhinav > > On 2021-05-10 21:20, Bjorn Andersson wrote: > > The first patch in the series is somewhat unrelated to the support, but > > simplifies reasoning and debugging of timing related issues. > > > > The second patch introduces support for dealing with different register > > block > > layouts, which is used in the forth patch to describe the hardware > > blocks found > > in the SC8180x eDP block. > > > > The third patch configures the INTF_CONFIG register, which carries the > > configuration for widebus handling. As with the DPU the bootloader > > enables > > widebus and we need to disable it, or implement support for adjusting > > the > > timing. > > > > Bjorn Andersson (4): > > drm/msm/dp: Simplify the mvid/nvid calculation > > drm/msm/dp: Store each subblock in the io region > > drm/msm/dp: Initialize the INTF_CONFIG register > > drm/msm/dp: Add support for SC8180x eDP > > > > drivers/gpu/drm/msm/dp/dp_catalog.c | 99 +++++++---------------------- > > drivers/gpu/drm/msm/dp/dp_display.c | 1 + > > drivers/gpu/drm/msm/dp/dp_parser.c | 22 +++++++ > > drivers/gpu/drm/msm/dp/dp_parser.h | 8 +++ > > 4 files changed, 53 insertions(+), 77 deletions(-)
Hi Bjorn On 2021-05-19 07:51, Bjorn Andersson wrote: > On Tue 18 May 22:41 CDT 2021, abhinavk@codeaurora.org wrote: > >> Hi Bjorn >> >> I had a quick glance on the series and before getting to other things >> wanted >> to know how you are initializing two different connectors for >> DP & EDP resp. >> >> The connector type for DP should be DRM_MODE_CONNECTOR_DisplayPort and >> eDP >> should be DRM_MODE_CONNECTOR_eDP. > > As far as I've been able to conclude there is no eDP support in the > upstream DPU driver; an encoder of type DRM_MODE_ENCODER_TMDS will only > attach to INTF_DP. > >> We need both to be created so that both EDP and DP can be supported >> concurrently. >> > > Further more the DP controller driver has a global variable to track > state and the INTF-picker will always pick the interface of index 0 > when > setting up the DP controller. > >> Will these changes work for concurrent eDP and DP case? >> > > The proposed changes are all that I need to get eDP working on my > sc8180x laptop. But the DPU code does not currently support more than a > single DP interface - and that has to be on the first INTF_DP that the > DPU driver knows about. > > But this is a limitation we should fix, rather than claiming that you > can only have one of each. Further more, afaict the sc7280 DP > controller > can do both DP and eDP, so it would make sense not to distinguish the > interfaces as eDP or DP - just because the product in mind will use > eDP. > > > PS. I've currently disabled the eDP interface on my laptop and am > working on trying to get Type-C DP working. Once that's in place I'd > need a better INTF/encoder picker - because the current model of just > picking INTF_DP 0 (or in a sequential fashion) won't work. > > Regards, > Bjorn Yes, we should be able to support eDP + DP concurrently on both sc7280 and sc8180x. Some of the changes we will be posting in the coming weeks should add support for it. Till then, as we spoke on IRC, since your changes dont break existing DP functionality, will continue reviewing rest of the changes. > >> Thanks >> >> Abhinav >> >> On 2021-05-10 21:20, Bjorn Andersson wrote: >> > The first patch in the series is somewhat unrelated to the support, but >> > simplifies reasoning and debugging of timing related issues. >> > >> > The second patch introduces support for dealing with different register >> > block >> > layouts, which is used in the forth patch to describe the hardware >> > blocks found >> > in the SC8180x eDP block. >> > >> > The third patch configures the INTF_CONFIG register, which carries the >> > configuration for widebus handling. As with the DPU the bootloader >> > enables >> > widebus and we need to disable it, or implement support for adjusting >> > the >> > timing. >> > >> > Bjorn Andersson (4): >> > drm/msm/dp: Simplify the mvid/nvid calculation >> > drm/msm/dp: Store each subblock in the io region >> > drm/msm/dp: Initialize the INTF_CONFIG register >> > drm/msm/dp: Add support for SC8180x eDP >> > >> > drivers/gpu/drm/msm/dp/dp_catalog.c | 99 +++++++---------------------- >> > drivers/gpu/drm/msm/dp/dp_display.c | 1 + >> > drivers/gpu/drm/msm/dp/dp_parser.c | 22 +++++++ >> > drivers/gpu/drm/msm/dp/dp_parser.h | 8 +++ >> > 4 files changed, 53 insertions(+), 77 deletions(-)
Hi Bjorn On 2021-05-10 21:20, Bjorn Andersson wrote: > In the search for causes to timing issues seen during implementation of > eDP support for SC8180x a fair amount of time was spent concluding why > the calculated mvid/nvid values where wrong. > > The overall conclusion is that the ratio of MVID/NVID describes, and > should match, the ratio between the pixel and link clock. > > Downstream this calculation reads the M and N values off the pixel > clock > straight from DISP_CC and are then adjusted based on knowledge of how > the link and vco_div (parent of the pixel clock) are derrived from the > common VCO. > > While upstreaming, and then extracting the PHY driver, the resulting > function performs the following steps: > > 1) Adjust the passed link rate based on the VCO divider used in the PHY > driver, and multiply this by 10 based on the link rate divider. > 2) Pick reasonable choices of M and N, by calculating the ratio between > this new clock and the pixel clock. > 3) Subtract M from N and flip the bits, to match the encoding of the N > register in DISP_CC. > 4) Flip the bits of N and add M, to get the value of N back. > 5) Multiply M with 5, per the documentation. > 6) Scale the values such that N is close to 0x8000 (or larger) > 7) Multply M with 2 or 3 depending on the link rate of HBR2 or HBR3. > > Presumably step 3) was added to provide step 4) with expected input, so > the two cancel each other out. The factor of 10 from step 1) goes into > the denominator and is partially cancelled by the 5 in the numerator in > step 5), resulting in step 7) simply cancelling out step 1). > Both the multiplication of M with 5 and N with 2 or 3 is coming because of the ratio between the vco clk and the link clk. So we could have 2.7, 5.4 or 8.1 Gbps link clks and the factor of 2 or 3 gets added because hbr2 is 2 * hbr and hbr3 is 3 * hbr. Your summary is pretty much right otherwise. Let me add some more points here: 1) Originally we removed reading the M_VID and N_VID from the DISPCC regs because of previous upstream comments that we can potentially just recalculate whatever the clk driver is programming by using rational_best_approximation https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/clk/qcom/clk-rcg2.c#L1160 Not having to read from DISPCC register is also useful because we dont have to maintain the register offset of the M_VID and N_VID which keeps changing across chipsets. However we discussed this again after viewing this patch. So the clk driver always operates on the vco clk and calculates the pixel clk from it and sets the M_VID and N_VID based on that. In terms of accuracy, the best way is still to re-use the M_VID and N_VID which the clk driver sets because the pixel clock was generated based on that and that is the actual pixel clock we are going to get. So even before this change we lost some accuracy because the pixel clock we are giving here to recalculate the M_VID and N_VID is a theoretical value. Although for most values of pixel clk, theoretical and actual should match. There could be corner cases of pixel clock where its a bit different. Hence ideally, re-using the M_VID and N_VID which the clk driver set would have been the best but not having to hard-code M_VID and N_VID offsets was a good enough reason to not go back to that again. Now, coming to this change. Here its trying to again re-calculate the M_VID and N_VID by using the same API which the clk driver uses but uses link clk and pixel clk as the parameters Vs the clk driver uses vco clk and actual pixel clock to calculate this. So even though this cleanup eliminates the adjustments we need to make to account for the VCO clk to link clk ratio, it also could bring additional difference between what was actually set by the clk driver and what we are calculating here because clk driver used vco clk as the input vs here we use link clk after this change. There might be some pixel clock rates of some resolutions where this difference could be risky. Hence the overall conclusion here was to keep using vco clk as the input to rational_best_approximation and not make more changes to this. > Left is the code that finds the ratio between the two arguments, scaled > to keep the denominator close to or larger than 0x8000. And this is our > mvid/nvid pair. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/gpu/drm/msm/dp/dp_catalog.c | 41 +++++------------------------ > 1 file changed, 6 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c > b/drivers/gpu/drm/msm/dp/dp_catalog.c > index b1a9b1b98f5f..2eb37ee48e42 100644 > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c > @@ -415,39 +415,16 @@ void dp_catalog_ctrl_config_msa(struct > dp_catalog *dp_catalog, > u32 rate, u32 stream_rate_khz, > bool fixed_nvid) > { > - u32 pixel_m, pixel_n; > - u32 mvid, nvid, pixel_div = 0, dispcc_input_rate; > u32 const nvid_fixed = DP_LINK_CONSTANT_N_VALUE; > - u32 const link_rate_hbr2 = 540000; > - u32 const link_rate_hbr3 = 810000; > - unsigned long den, num; > - > + unsigned long mvid, nvid; > struct dp_catalog_private *catalog = container_of(dp_catalog, > struct dp_catalog_private, dp_catalog); > > - if (rate == link_rate_hbr3) > - pixel_div = 6; > - else if (rate == 1620000 || rate == 270000) > - pixel_div = 2; > - else if (rate == link_rate_hbr2) > - pixel_div = 4; > - else > - DRM_ERROR("Invalid pixel mux divider\n"); > - > - dispcc_input_rate = (rate * 10) / pixel_div; > - > - rational_best_approximation(dispcc_input_rate, stream_rate_khz, > - (unsigned long)(1 << 16) - 1, > - (unsigned long)(1 << 16) - 1, &den, &num); > - > - den = ~(den - num); > - den = den & 0xFFFF; > - pixel_m = num; > - pixel_n = den; > - > - mvid = (pixel_m & 0xFFFF) * 5; > - nvid = (0xFFFF & (~pixel_n)) + (pixel_m & 0xFFFF); > + rational_best_approximation(stream_rate_khz, rate, > + (1 << 16) - 1, (1 << 16) - 1, > + &mvid, &nvid); > > + /* Adjust values so that nvid is close to DP_LINK_CONSTANT_N_VALUE */ > if (nvid < nvid_fixed) { > u32 temp; > > @@ -456,13 +433,7 @@ void dp_catalog_ctrl_config_msa(struct dp_catalog > *dp_catalog, > nvid = temp; > } > > - if (link_rate_hbr2 == rate) > - nvid *= 2; > - > - if (link_rate_hbr3 == rate) > - nvid *= 3; > - > - DRM_DEBUG_DP("mvid=0x%x, nvid=0x%x\n", mvid, nvid); > + DRM_DEBUG_DP("mvid=0x%lx, nvid=0x%lx\n", mvid, nvid); > dp_write_link(catalog, REG_DP_SOFTWARE_MVID, mvid); > dp_write_link(catalog, REG_DP_SOFTWARE_NVID, nvid); > dp_write_p0(catalog, MMSS_DP_DSC_DTO, 0x0);