[Freedreno] [PATCH] drm/msm/dp: Shorten SETUP timeout
Found in the middle of a patch from Sankeerth was the reduction of the INIT_SETUP timeout from 10s to 100ms. Upon INIT_SETUP timeout the host is initalized and HPD interrupt start to be serviced, so in the case of eDP this reduction improves the user experience dramatically - i.e. removes 9.9s of bland screen time at boot. Suggested-by: Sankeerth Billakanti Signed-off-by: Bjorn Andersson --- drivers/gpu/drm/msm/dp/dp_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 21b9c1de4ecb..46d9f3eb6d13 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1438,7 +1438,7 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display) dp_hpd_event_setup(dp); - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100); + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 1); } void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor) -- 2.29.2
[Freedreno] [RFC] drm/msm/dp: Add typec_mux implementation
Implement a typec_mux in order to allow a Type-C controller to signal
the connection and attention of DisplayPort to the related USB-C port.
The remains of support for something along this lines was left in
the dp_display as the driver was upstreamed, so these are reused with
minimal modifications necessary.
When operating in this mode, HPD interrupts has still been observed in
the ISR so, in line with the downstream kernel, these are ignored.
Signed-off-by: Bjorn Andersson
---
This applies on top of
https://lore.kernel.org/linux-arm-msm/20211001180058.1021913-1-bjorn.anders...@linaro.org/
drivers/gpu/drm/msm/Kconfig | 1 +
drivers/gpu/drm/msm/dp/dp_display.c | 52 ---
drivers/gpu/drm/msm/dp/dp_hpd.c | 54 +
3 files changed, 87 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index 5879f67bc88c..4e4b98c448cb 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -9,6 +9,7 @@ config DRM_MSM
depends on QCOM_OCMEM || QCOM_OCMEM=n
depends on QCOM_LLCC || QCOM_LLCC=n
depends on QCOM_COMMAND_DB || QCOM_COMMAND_DB=n
+ depends on TYPEC || TYPEC=n
select IOMMU_IO_PGTABLE
select QCOM_MDT_LOADER if ARCH_QCOM
select REGULATOR
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 56a79aeffed4..e863f537047a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -85,6 +85,8 @@ struct dp_display_private {
bool hpd_irq_on;
bool audio_supported;
+ bool use_hw_hpd;
+
struct platform_device *pdev;
struct dentry *root;
@@ -466,11 +468,10 @@ static int dp_display_handle_irq_hpd(struct
dp_display_private *dp)
return 0;
}
-static int dp_display_usbpd_attention_cb(struct device *dev)
+static int dp_display_usbpd_attention(struct dp_display_private *dp)
{
int rc = 0;
u32 sink_request;
- struct dp_display_private *dp = dev_get_dp_display_private(dev);
/* check for any test request issued by sink */
rc = dp_link_process_request(dp->link);
@@ -690,7 +691,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp,
u32 data)
return 0;
}
- ret = dp_display_usbpd_attention_cb(>pdev->dev);
+ ret = dp_display_usbpd_attention(dp);
if (ret == -ECONNRESET) { /* cable unplugged */
dp->core_initialized = false;
}
@@ -709,6 +710,13 @@ static void dp_display_deinit_sub_modules(struct
dp_display_private *dp)
dp_audio_put(dp->audio);
}
+static int dp_display_usbpd_attention_cb(struct device *dev)
+{
+ struct dp_display_private *dp = dev_get_dp_display_private(dev);
+
+ return dp_irq_hpd_handle(dp, 0);
+}
+
static int dp_init_sub_modules(struct dp_display_private *dp)
{
int rc = 0;
@@ -731,6 +739,8 @@ static int dp_init_sub_modules(struct dp_display_private
*dp)
goto error;
}
+ dp->use_hw_hpd = !of_property_read_bool(dev->of_node, "mode-switch");
+
dp->parser = dp_parser_get(dp->pdev);
if (IS_ERR(dp->parser)) {
rc = PTR_ERR(dp->parser);
@@ -1135,27 +1145,29 @@ static irqreturn_t dp_display_irq_handler(int irq, void
*dev_id)
return IRQ_NONE;
}
- hpd_isr_status = dp_catalog_hpd_get_intr_status(dp->catalog);
+ if (dp->use_hw_hpd) {
+ hpd_isr_status = dp_catalog_hpd_get_intr_status(dp->catalog);
- DRM_DEBUG_DP("hpd isr status=%#x\n", hpd_isr_status);
- if (hpd_isr_status & 0x0F) {
- /* hpd related interrupts */
- if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK)
- dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
+ DRM_DEBUG_DP("hpd isr status=%#x\n", hpd_isr_status);
+ if (hpd_isr_status & 0x0F) {
+ /* hpd related interrupts */
+ if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK)
+ dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
- if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK) {
- /* stop sentinel connect pending checking */
- dp_del_event(dp, EV_CONNECT_PENDING_TIMEOUT);
- dp_add_event(dp, EV_IRQ_HPD_INT, 0, 0);
- }
+ if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK) {
+ /* stop sentinel connect pending checking */
+ dp_del_event(dp, EV_CONNECT_PENDING_TIMEOUT);
+ dp_add_event(dp, EV_IRQ_HPD_INT, 0, 0);
+ }
- if (hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK) {
- dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
- dp_add_event(dp, EV_HPD_PLUG_INT, 0,
Re: [Freedreno] [RFC] drm/msm/dp: Allow attaching a drm_panel
On Mon 04 Oct 20:50 CDT 2021, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2021-10-04 18:11:11)
> > On Mon 04 Oct 17:36 PDT 2021, Doug Anderson wrote:
> >
> > > Hi,
> > >
> > > On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
> > > wrote:
> > > >
> > > > On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > > > > wrote:
> > > > > >
> > > > > > +static int dp_parser_find_panel(struct dp_parser *parser)
> > > > > > +{
> > > > > > + struct device_node *np = parser->pdev->dev.of_node;
> > > > > > + int rc;
> > > > > > +
> > > > > > + rc = drm_of_find_panel_or_bridge(np, 2, 0,
> > > > > > >drm_panel, NULL);
> > > > >
> > > > > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > > > > port 1 is "Output endpoint of the controller". We should just use port
> > > > > 1 here, right?
> > > > >
> > > >
> > > > Finally got back to this, changed it to 1 and figured out why I left it
> > > > at 2.
> > > >
> > > > drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
> > > > reference to the USB-C controller, scan through the registered panels
> > > > and conclude that the of_node of the USB-C controller isn't a registered
> > > > panel and return -EPROBE_DEFER.
> > >
> > > I'm confused, but maybe it would help if I could see something
> > > concrete. Is there a specific board this was happening on?
> > >
> >
> > Right, let's make this more concrete with a snippet from the actual
> > SC8180x DT.
>
> Where is this DT? Is it in the kernel tree?
>
Still missing a bunch of driver pieces, so I haven't yet pushed any of
this upstream.
But if you're interested you can find some work-in-progress here:
https://github.com/andersson/kernel/commits/wip/sc8180x-next-20210819
> >
> > > Under the DP node in the device tree I expect:
> > >
> > > ports {
> > > port@1 {
> > > reg = <1>;
> > > edp_out: endpoint {
> > > remote-endpoint = <_panel_in>;
> > > };
> > > };
> > > };
> > >
> >
> > /* We got a panel */
> > panel {
> > ...
> > ports {
> > port {
> > auo_b133han05_in: endpoint {
> > remote-endpoint = <_edp_out>;
> > };
> > };
> > };
> > };
> >
> > /* And a 2-port USB-C controller */
> > type-c-controller {
> > ...
> > connector@0 {
> > ports {
> > port@0 {
> > reg = <0>;
> > ucsi_port_0_dp: endpoint {
> > remote-endpoint = <_mode>;
> > };
> > };
> >
> > port@1 {
> > reg = <1>;
> > ucsi_port_0_switch: endpoint {
> > remote-endpoint = <_qmp_phy>;
> > };
> > };
> > };
> > };
> >
> > connector@1 {
> > ports {
> > port@0 {
> > reg = <0>;
> > ucsi_port_1_dp: endpoint {
> > remote-endpoint = <_mode>;
> > };
> > };
> >
> > port@1 {
> > reg = <1>;
> > ucsi_port_1_switch: endpoint {
> > remote-endpoint = <_qmp_phy>;
> > };
> > };
> > };
> > };
> > };
> >
> > /* And then our 2 DP and single eDP controllers */
> > _dp0 {
> > ports {
> > port@1 {
> > reg = <1>;
> > dp0_mode: endpoint {
> > remote-endpoint = <_port_0_dp>;
> > };
> > };
> > };
> > };
> >
> > _dp1 {
> > ports {
> > port@1 {
> > reg = <1>;
> > dp1_mode: endpoint {
> > remote-endpoint = <_port_1_dp>;
> > };
> > };
> > };
> > };
> >
> > _edp {
> > ports {
> > port@1 {
> > reg = <1>;
> > mdss_edp_out: endpoint {
> > remote-endpoint = <_b133han05_in>;
> > };
> > };
> > };
> > };
> >
> > > If you have "port@1" pointing to a USB-C controller but this instance
> > > of the DP controller is actually hooked up straight to a panel then
> > > you should simply delete the "port@1" that points to the typeC and
> > > replace it with one that points to a panel, right?
> > >
> >
> > As you can see, port 1 on _dp0 and _dp1 points to the two UCSI
> > connectors and the eDP points to the panel, exactly like we agreed.
> >
> > So now I call:
> > drm_of_find_panel_or_bridge(dev->of_node, 1, 0, , NULL);
> >
> > which for the two DP nodes will pass respective UCSI connector to
> > drm_find_panel() and get EPROBE_DEFER back - because they are not on
> > panel_list.
>
> That's "good" right?
>
Well, it's expected that the connectors aren't panels...
> >
> > There's nothing indicating in the of_graph that the USB connectors
> > aren't panels (or bridges), so I don't see a way to distinguish the two
> > types remotes.
Re: [Freedreno] [PATCH v1 2/4] arm64: dts: qcom: sc7280: add display dt nodes
Quoting mkri...@codeaurora.org (2021-09-30 23:39:07)
> On 2021-09-30 23:28, Stephen Boyd wrote:
> > Quoting mkri...@codeaurora.org (2021-09-30 04:56:59)
> >> On 2021-08-19 01:27, Stephen Boyd wrote:
> >> > Quoting Krishna Manikandan (2021-08-18 03:27:02)
> >> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >> >> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >> >> index 53a21d0..fd7ff1c 100644
> >> >> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >> >> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >> >> +
> >> >> + status = "disabled";
> >> >> +
> >> >> + mdp: mdp@ae01000 {
> >> >
> >> > display-controller@ae01000
> >>
> >> Stephen,
> >> In the current driver code, there is a substring comparison for
> >> "mdp"
> >> in device node name as part of probe sequence. If "mdp" is not present
> >> in the node name, it will
> >> return an error resulting in probe failure. Can we continue using
> >> mdp
> >> as nodename instead of display controller?
> >>
> >
> > Can we fix the driver to not look for node names and look for
> > compatible
> > strings instead? It took me a minute to find compare_name_mdp() in
> > drivers/gpu/drm/msm/msm_drv.c to understand what you're talking about.
> > Perhaps looking for qcom,mdp5 in there will be sufficient instead of
> > looking at the node name.
>
> Sure Stephen. I will make the necessary changes in msm_drv.c to look for
> compatible string instead of node name.
> Can I include these two changes (changing mdp--> display controller and
> msm_drv.c changes) in a separate series ?
>
Sure. So you'll send the drm driver change now and we'll get the DT
change after that with the more generic node name?
Re: [Freedreno] [RFC] drm/msm/dp: Allow attaching a drm_panel
Quoting Bjorn Andersson (2021-10-04 18:11:11)
> On Mon 04 Oct 17:36 PDT 2021, Doug Anderson wrote:
>
> > Hi,
> >
> > On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
> > wrote:
> > >
> > > On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> > >
> > > > Hi,
> > > >
> > > > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > > > wrote:
> > > > >
> > > > > +static int dp_parser_find_panel(struct dp_parser *parser)
> > > > > +{
> > > > > + struct device_node *np = parser->pdev->dev.of_node;
> > > > > + int rc;
> > > > > +
> > > > > + rc = drm_of_find_panel_or_bridge(np, 2, 0,
> > > > > >drm_panel, NULL);
> > > >
> > > > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > > > port 1 is "Output endpoint of the controller". We should just use port
> > > > 1 here, right?
> > > >
> > >
> > > Finally got back to this, changed it to 1 and figured out why I left it
> > > at 2.
> > >
> > > drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
> > > reference to the USB-C controller, scan through the registered panels
> > > and conclude that the of_node of the USB-C controller isn't a registered
> > > panel and return -EPROBE_DEFER.
> >
> > I'm confused, but maybe it would help if I could see something
> > concrete. Is there a specific board this was happening on?
> >
>
> Right, let's make this more concrete with a snippet from the actual
> SC8180x DT.
Where is this DT? Is it in the kernel tree?
>
> > Under the DP node in the device tree I expect:
> >
> > ports {
> > port@1 {
> > reg = <1>;
> > edp_out: endpoint {
> > remote-endpoint = <_panel_in>;
> > };
> > };
> > };
> >
>
> /* We got a panel */
> panel {
> ...
> ports {
> port {
> auo_b133han05_in: endpoint {
> remote-endpoint = <_edp_out>;
> };
> };
> };
> };
>
> /* And a 2-port USB-C controller */
> type-c-controller {
> ...
> connector@0 {
> ports {
> port@0 {
> reg = <0>;
> ucsi_port_0_dp: endpoint {
> remote-endpoint = <_mode>;
> };
> };
>
> port@1 {
> reg = <1>;
> ucsi_port_0_switch: endpoint {
> remote-endpoint = <_qmp_phy>;
> };
> };
> };
> };
>
> connector@1 {
> ports {
> port@0 {
> reg = <0>;
> ucsi_port_1_dp: endpoint {
> remote-endpoint = <_mode>;
> };
> };
>
> port@1 {
> reg = <1>;
> ucsi_port_1_switch: endpoint {
> remote-endpoint = <_qmp_phy>;
> };
> };
> };
> };
> };
>
> /* And then our 2 DP and single eDP controllers */
> _dp0 {
> ports {
> port@1 {
> reg = <1>;
> dp0_mode: endpoint {
> remote-endpoint = <_port_0_dp>;
> };
> };
> };
> };
>
> _dp1 {
> ports {
> port@1 {
> reg = <1>;
> dp1_mode: endpoint {
> remote-endpoint = <_port_1_dp>;
> };
> };
> };
> };
>
> _edp {
> ports {
> port@1 {
> reg = <1>;
> mdss_edp_out: endpoint {
> remote-endpoint = <_b133han05_in>;
> };
> };
> };
> };
>
> > If you have "port@1" pointing to a USB-C controller but this instance
> > of the DP controller is actually hooked up straight to a panel then
> > you should simply delete the "port@1" that points to the typeC and
> > replace it with one that points to a panel, right?
> >
>
> As you can see, port 1 on _dp0 and _dp1 points to the two UCSI
> connectors and the eDP points to the panel, exactly like we agreed.
>
> So now I call:
> drm_of_find_panel_or_bridge(dev->of_node, 1, 0, , NULL);
>
> which for the two DP nodes will pass respective UCSI connector to
> drm_find_panel() and get EPROBE_DEFER back - because they are not on
> panel_list.
That's "good" right?
>
> There's nothing indicating in the of_graph that the USB connectors
> aren't panels (or bridges), so I don't see a way to distinguish the two
> types remotes.
>
I'd like to create a bridge, not panel, for USB connectors, so that we
can push sideband HPD signaling through to the DP driver. But either way
this should work, right? If drm_of_find_panel_or_bridge() returns
-EPROBE_DEFER, then assume the connector is DP. Otherwise if there's a
valid pointer then treat it as eDP. We can't go too crazy though because
once we attach a bridge we're assuming eDP which may not actually be
true.
If we make a bridge for type-C USB connectors then we'll be able to use
the drm_bridge_connector code to automatically figure out the connector
type (eDP vs. DP vs. whatever else is chained onto the end of the DP
connector). That
Re: [Freedreno] [PATCH v3 3/5] drm/msm/dp: Support up to 3 DP controllers
Quoting Bjorn Andersson (2021-10-04 18:15:20)
> On Mon 04 Oct 17:58 PDT 2021, Stephen Boyd wrote:
>
> > Quoting Bjorn Andersson (2021-10-01 11:00:56)
> > > Based on the removal of the g_dp_display and the movement of the
> > > priv->dp lookup into the DP code it's now possible to have multiple
> > > DP instances.
> > >
> > > In line with the other controllers in the MSM driver, introduce a
> > > per-compatible list of base addresses which is used to resolve the
> > > "instance id" for the given DP controller. This instance id is used as
> > > index in the priv->dp[] array.
> > >
> > > Then extend the initialization code to initialize struct drm_encoder for
> > > each of the registered priv->dp[] and update the logic for associating
> > > each struct msm_dp with the struct dpu_encoder_virt.
> > >
> > > Lastly, bump the number of struct msm_dp instances carries by priv->dp
> > > to 3, the currently known maximum number of controllers found in a
> > > Qualcomm SoC.
> > >
> > > Signed-off-by: Bjorn Andersson
> > > ---
> >
> > Reviewed-by: Stephen Boyd
> >
> > Some nits below.
> >
> > >
> > > Changes since v2:
> > > - Added MSM_DRM_DP_COUNT to link the two 3s
> > > - Moved NULL check for msm_dp_debugfs_init() to the call site
> > > - Made struct dp_display_private->id unsigned
> > >
> > > I also implemented added connector_type to each of the DP instances and
> > > propagated this to dp_drm_connector_init() but later dropped this again
> > > per
> > > Doug's suggestion that we'll base this on the presence/absence of a
> > > associated
> > > drm bridge or panel.
> >
> > Sad but OK. We can take up that topic in another patch.
> >
>
> So you don't agree with the solution from sn65dsi86?
>
> The only reason I haven't yet send this other patch is the of_graph
> thing Doug an I are discussing on the RFC. But if we agree to base this
> on compatible we could decide to look only for panels for the edp
> instances and avoid that problem...
>
> We would however never be able to describe the USB-less DP instance with
> a panel explicitly described in DT going that route.
I'll reply on that thread.
>
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > index f655adbc2421..875b07e7183d 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > > @@ -203,8 +204,10 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms,
> > > struct drm_minor *minor)
> > > dpu_debugfs_vbif_init(dpu_kms, entry);
> > > dpu_debugfs_core_irq_init(dpu_kms, entry);
> > >
> > > - if (priv->dp)
> > > - msm_dp_debugfs_init(priv->dp, minor);
> > > + for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
> > > + if (priv->dp[i])
> > > + msm_dp_debugfs_init(priv->dp[i], minor);
> >
> > This seems to cause a bunch of debugfs warnings when there are multiple
> > nodes created with the same name.
> >
>
> Yes, that's true. I have a half-baked follow up that attempts to create
> instance-specific debugfs directories. Can we take that in a follow up?
Sure.
>
> > > + }
> > >
> > > return dpu_core_perf_debugfs_init(dpu_kms, entry);
> > > }
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > > b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index 5d3ee5ef07c2..ff3477474c5d 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -1180,10 +1192,31 @@ int dp_display_request_irq(struct msm_dp
> > > *dp_display)
> > > return 0;
> > > }
> > >
> > > +static int dp_display_find_id(struct platform_device *pdev)
> > > +{
> > > + const struct msm_dp_config *cfg =
> > > of_device_get_match_data(>dev);
> > > + struct resource *res;
> > > + int i;
> > > +
> > > +
> >
> > Nitpick: Remove a newline here.
> >
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + if (!res)
> > > + return -EINVAL;
> > > +
> > > + for (i = 0; i < cfg->num_descs; i++) {
> > > + if (cfg->io_start[i] == res->start)
> > > + return i;
> > > + }
> >
> > Nitpick: Drop braces on single line if inside for loop.
> >
>
> Not when the loop spans multiple lines?
Kernel style is to remove braces from single "statement" for loops where
in this case the statement is the if condition.
>
> > > +
> > > + dev_err(>dev, "unknown displayport instance\n");
> > > + return -EINVAL;
> > > +}
> > > +
Re: [Freedreno] [PATCH v3 3/5] drm/msm/dp: Refactor ioremap wrapper
On Mon 04 Oct 18:04 PDT 2021, Stephen Boyd wrote: > Quoting Bjorn Andersson (2021-10-01 10:43:58) > > In order to deal with multiple memory ranges in the following commit > > change the ioremap wrapper to not poke directly into the dss_io_data > > struct. > > > > While at it, devm_ioremap_resource() already prints useful error > > messages on failure, so omit the unnecessary prints from the caller. > > > > Signed-off-by: Bjorn Andersson > > --- > > Reviewed-by: Stephen Boyd > > I realize this will cause some prints when we use old DTs. I suppose > that's OK though because we'll have more incentive to update existing > DT. The use of the current binding is fairly limited, so I think that makes sense. Abhinav also requested earlier that we do that and drop the fallback sooner rather than later, which I would like to see as well. Thanks, Bjorn
Re: [Freedreno] [PATCH v3 3/5] drm/msm/dp: Support up to 3 DP controllers
On Mon 04 Oct 17:58 PDT 2021, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2021-10-01 11:00:56)
> > Based on the removal of the g_dp_display and the movement of the
> > priv->dp lookup into the DP code it's now possible to have multiple
> > DP instances.
> >
> > In line with the other controllers in the MSM driver, introduce a
> > per-compatible list of base addresses which is used to resolve the
> > "instance id" for the given DP controller. This instance id is used as
> > index in the priv->dp[] array.
> >
> > Then extend the initialization code to initialize struct drm_encoder for
> > each of the registered priv->dp[] and update the logic for associating
> > each struct msm_dp with the struct dpu_encoder_virt.
> >
> > Lastly, bump the number of struct msm_dp instances carries by priv->dp
> > to 3, the currently known maximum number of controllers found in a
> > Qualcomm SoC.
> >
> > Signed-off-by: Bjorn Andersson
> > ---
>
> Reviewed-by: Stephen Boyd
>
> Some nits below.
>
> >
> > Changes since v2:
> > - Added MSM_DRM_DP_COUNT to link the two 3s
> > - Moved NULL check for msm_dp_debugfs_init() to the call site
> > - Made struct dp_display_private->id unsigned
> >
> > I also implemented added connector_type to each of the DP instances and
> > propagated this to dp_drm_connector_init() but later dropped this again per
> > Doug's suggestion that we'll base this on the presence/absence of a
> > associated
> > drm bridge or panel.
>
> Sad but OK. We can take up that topic in another patch.
>
So you don't agree with the solution from sn65dsi86?
The only reason I haven't yet send this other patch is the of_graph
thing Doug an I are discussing on the RFC. But if we agree to base this
on compatible we could decide to look only for panels for the edp
instances and avoid that problem...
We would however never be able to describe the USB-less DP instance with
a panel explicitly described in DT going that route.
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index f655adbc2421..875b07e7183d 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -203,8 +204,10 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms,
> > struct drm_minor *minor)
> > dpu_debugfs_vbif_init(dpu_kms, entry);
> > dpu_debugfs_core_irq_init(dpu_kms, entry);
> >
> > - if (priv->dp)
> > - msm_dp_debugfs_init(priv->dp, minor);
> > + for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
> > + if (priv->dp[i])
> > + msm_dp_debugfs_init(priv->dp[i], minor);
>
> This seems to cause a bunch of debugfs warnings when there are multiple
> nodes created with the same name.
>
Yes, that's true. I have a half-baked follow up that attempts to create
instance-specific debugfs directories. Can we take that in a follow up?
> > + }
> >
> > return dpu_core_perf_debugfs_init(dpu_kms, entry);
> > }
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 5d3ee5ef07c2..ff3477474c5d 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -1180,10 +1192,31 @@ int dp_display_request_irq(struct msm_dp
> > *dp_display)
> > return 0;
> > }
> >
> > +static int dp_display_find_id(struct platform_device *pdev)
> > +{
> > + const struct msm_dp_config *cfg =
> > of_device_get_match_data(>dev);
> > + struct resource *res;
> > + int i;
> > +
> > +
>
> Nitpick: Remove a newline here.
>
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + return -EINVAL;
> > +
> > + for (i = 0; i < cfg->num_descs; i++) {
> > + if (cfg->io_start[i] == res->start)
> > + return i;
> > + }
>
> Nitpick: Drop braces on single line if inside for loop.
>
Not when the loop spans multiple lines?
> > +
> > + dev_err(>dev, "unknown displayport instance\n");
> > + return -EINVAL;
> > +}
> > +
Re: [Freedreno] [RFC] drm/msm/dp: Allow attaching a drm_panel
On Mon 04 Oct 17:36 PDT 2021, Doug Anderson wrote:
> Hi,
>
> On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
> wrote:
> >
> > On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
> >
> > > Hi,
> > >
> > > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > > wrote:
> > > >
> > > > +static int dp_parser_find_panel(struct dp_parser *parser)
> > > > +{
> > > > + struct device_node *np = parser->pdev->dev.of_node;
> > > > + int rc;
> > > > +
> > > > + rc = drm_of_find_panel_or_bridge(np, 2, 0, >drm_panel,
> > > > NULL);
> > >
> > > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > > port 1 is "Output endpoint of the controller". We should just use port
> > > 1 here, right?
> > >
> >
> > Finally got back to this, changed it to 1 and figured out why I left it
> > at 2.
> >
> > drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
> > reference to the USB-C controller, scan through the registered panels
> > and conclude that the of_node of the USB-C controller isn't a registered
> > panel and return -EPROBE_DEFER.
>
> I'm confused, but maybe it would help if I could see something
> concrete. Is there a specific board this was happening on?
>
Right, let's make this more concrete with a snippet from the actual
SC8180x DT.
> Under the DP node in the device tree I expect:
>
> ports {
> port@1 {
> reg = <1>;
> edp_out: endpoint {
> remote-endpoint = <_panel_in>;
> };
> };
> };
>
/* We got a panel */
panel {
...
ports {
port {
auo_b133han05_in: endpoint {
remote-endpoint = <_edp_out>;
};
};
};
};
/* And a 2-port USB-C controller */
type-c-controller {
...
connector@0 {
ports {
port@0 {
reg = <0>;
ucsi_port_0_dp: endpoint {
remote-endpoint = <_mode>;
};
};
port@1 {
reg = <1>;
ucsi_port_0_switch: endpoint {
remote-endpoint = <_qmp_phy>;
};
};
};
};
connector@1 {
ports {
port@0 {
reg = <0>;
ucsi_port_1_dp: endpoint {
remote-endpoint = <_mode>;
};
};
port@1 {
reg = <1>;
ucsi_port_1_switch: endpoint {
remote-endpoint = <_qmp_phy>;
};
};
};
};
};
/* And then our 2 DP and single eDP controllers */
_dp0 {
ports {
port@1 {
reg = <1>;
dp0_mode: endpoint {
remote-endpoint = <_port_0_dp>;
};
};
};
};
_dp1 {
ports {
port@1 {
reg = <1>;
dp1_mode: endpoint {
remote-endpoint = <_port_1_dp>;
};
};
};
};
_edp {
ports {
port@1 {
reg = <1>;
mdss_edp_out: endpoint {
remote-endpoint = <_b133han05_in>;
};
};
};
};
> If you have "port@1" pointing to a USB-C controller but this instance
> of the DP controller is actually hooked up straight to a panel then
> you should simply delete the "port@1" that points to the typeC and
> replace it with one that points to a panel, right?
>
As you can see, port 1 on _dp0 and _dp1 points to the two UCSI
connectors and the eDP points to the panel, exactly like we agreed.
So now I call:
drm_of_find_panel_or_bridge(dev->of_node, 1, 0, , NULL);
which for the two DP nodes will pass respective UCSI connector to
drm_find_panel() and get EPROBE_DEFER back - because they are not on
panel_list.
There's nothing indicating in the of_graph that the USB connectors
aren't panels (or bridges), so I don't see a way to distinguish the two
types remotes.
Hope that clarifies my conundrum.
Regards,
Bjorn
Re: [Freedreno] [PATCH v3 3/5] drm/msm/dp: Refactor ioremap wrapper
Quoting Bjorn Andersson (2021-10-01 10:43:58) > In order to deal with multiple memory ranges in the following commit > change the ioremap wrapper to not poke directly into the dss_io_data > struct. > > While at it, devm_ioremap_resource() already prints useful error > messages on failure, so omit the unnecessary prints from the caller. > > Signed-off-by: Bjorn Andersson > --- Reviewed-by: Stephen Boyd I realize this will cause some prints when we use old DTs. I suppose that's OK though because we'll have more incentive to update existing DT.
Re: [Freedreno] [PATCH v3 3/5] drm/msm/dp: Support up to 3 DP controllers
Quoting Bjorn Andersson (2021-10-01 11:00:56)
> Based on the removal of the g_dp_display and the movement of the
> priv->dp lookup into the DP code it's now possible to have multiple
> DP instances.
>
> In line with the other controllers in the MSM driver, introduce a
> per-compatible list of base addresses which is used to resolve the
> "instance id" for the given DP controller. This instance id is used as
> index in the priv->dp[] array.
>
> Then extend the initialization code to initialize struct drm_encoder for
> each of the registered priv->dp[] and update the logic for associating
> each struct msm_dp with the struct dpu_encoder_virt.
>
> Lastly, bump the number of struct msm_dp instances carries by priv->dp
> to 3, the currently known maximum number of controllers found in a
> Qualcomm SoC.
>
> Signed-off-by: Bjorn Andersson
> ---
Reviewed-by: Stephen Boyd
Some nits below.
>
> Changes since v2:
> - Added MSM_DRM_DP_COUNT to link the two 3s
> - Moved NULL check for msm_dp_debugfs_init() to the call site
> - Made struct dp_display_private->id unsigned
>
> I also implemented added connector_type to each of the DP instances and
> propagated this to dp_drm_connector_init() but later dropped this again per
> Doug's suggestion that we'll base this on the presence/absence of a associated
> drm bridge or panel.
Sad but OK. We can take up that topic in another patch.
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index f655adbc2421..875b07e7183d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -203,8 +204,10 @@ static int dpu_kms_debugfs_init(struct msm_kms *kms,
> struct drm_minor *minor)
> dpu_debugfs_vbif_init(dpu_kms, entry);
> dpu_debugfs_core_irq_init(dpu_kms, entry);
>
> - if (priv->dp)
> - msm_dp_debugfs_init(priv->dp, minor);
> + for (i = 0; i < ARRAY_SIZE(priv->dp); i++) {
> + if (priv->dp[i])
> + msm_dp_debugfs_init(priv->dp[i], minor);
This seems to cause a bunch of debugfs warnings when there are multiple
nodes created with the same name.
> + }
>
> return dpu_core_perf_debugfs_init(dpu_kms, entry);
> }
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5d3ee5ef07c2..ff3477474c5d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1180,10 +1192,31 @@ int dp_display_request_irq(struct msm_dp *dp_display)
> return 0;
> }
>
> +static int dp_display_find_id(struct platform_device *pdev)
> +{
> + const struct msm_dp_config *cfg =
> of_device_get_match_data(>dev);
> + struct resource *res;
> + int i;
> +
> +
Nitpick: Remove a newline here.
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -EINVAL;
> +
> + for (i = 0; i < cfg->num_descs; i++) {
> + if (cfg->io_start[i] == res->start)
> + return i;
> + }
Nitpick: Drop braces on single line if inside for loop.
> +
> + dev_err(>dev, "unknown displayport instance\n");
> + return -EINVAL;
> +}
> +
Re: [Freedreno] [RFC] drm/msm/dp: Allow attaching a drm_panel
Hi,
On Fri, Oct 1, 2021 at 2:00 PM Bjorn Andersson
wrote:
>
> On Fri 27 Aug 13:52 PDT 2021, Doug Anderson wrote:
>
> > Hi,
> >
> > On Mon, Jul 26, 2021 at 4:15 PM Bjorn Andersson
> > wrote:
> > >
> > > +static int dp_parser_find_panel(struct dp_parser *parser)
> > > +{
> > > + struct device_node *np = parser->pdev->dev.of_node;
> > > + int rc;
> > > +
> > > + rc = drm_of_find_panel_or_bridge(np, 2, 0, >drm_panel,
> > > NULL);
> >
> > Why port 2? Shouldn't this just be port 1 always? The yaml says that
> > port 1 is "Output endpoint of the controller". We should just use port
> > 1 here, right?
> >
>
> Finally got back to this, changed it to 1 and figured out why I left it
> at 2.
>
> drm_of_find_panel_or_bridge() on a DP controller will find the of_graph
> reference to the USB-C controller, scan through the registered panels
> and conclude that the of_node of the USB-C controller isn't a registered
> panel and return -EPROBE_DEFER.
I'm confused, but maybe it would help if I could see something
concrete. Is there a specific board this was happening on?
Under the DP node in the device tree I expect:
ports {
port@1 {
reg = <1>;
edp_out: endpoint {
remote-endpoint = <_panel_in>;
};
};
};
If you have "port@1" pointing to a USB-C controller but this instance
of the DP controller is actually hooked up straight to a panel then
you should simply delete the "port@1" that points to the typeC and
replace it with one that points to a panel, right?
-Doug
Re: [Freedreno] [PATCH v3 03/14] drm/hdcp: Update property value on content type and user changes
On 2021-10-01 08:11, Sean Paul wrote:
From: Sean Paul
This patch updates the connector's property value in 2 cases which were
previously missed:
1- Content type changes. The value should revert back to DESIRED from
ENABLED in case the driver must re-authenticate the link due to the
new content type.
2- Userspace sets value to DESIRED while ENABLED. In this case, the
value should be reset immediately to ENABLED since the link is
actively being encrypted.
To accommodate these changes, I've split up the conditionals to make
things a bit more clear (as much as one can with this mess of state).
Acked-by: Jani Nikula
Signed-off-by: Sean Paul
Reviewed-by: Abhinav Kumar
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-4-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-4-s...@poorly.run
#v2
Changes in v2:
-None
Changes in v3:
-Fixed indentation issue identified by 0-day
---
drivers/gpu/drm/drm_hdcp.c | 26 +-
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c
index dd8fa91c51d6..8c851d40cd45 100644
--- a/drivers/gpu/drm/drm_hdcp.c
+++ b/drivers/gpu/drm/drm_hdcp.c
@@ -487,21 +487,29 @@ bool drm_hdcp_atomic_check(struct drm_connector
*connector,
return true;
/*
-* Nothing to do if content type is unchanged and one of:
-* - state didn't change
+* Content type changes require an HDCP disable/enable cycle.
+*/
+ if (new_conn_state->hdcp_content_type !=
old_conn_state->hdcp_content_type) {
+ new_conn_state->content_protection =
+ DRM_MODE_CONTENT_PROTECTION_DESIRED;
+ return true;
+ }
+
+ /*
+* Ignore meaningless state changes:
* - HDCP was activated since the last commit
-* - attempting to set to desired while already enabled
+* - Attempting to set to desired while already enabled
*/
- if (old_hdcp == new_hdcp ||
- (old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
+ if ((old_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
new_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) ||
(old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) {
- if (old_conn_state->hdcp_content_type ==
- new_conn_state->hdcp_content_type)
- return false;
+ new_conn_state->content_protection =
+ DRM_MODE_CONTENT_PROTECTION_ENABLED;
+ return false;
}
- return true;
+ /* Finally, if state changes, we need action */
+ return old_hdcp != new_hdcp;
}
EXPORT_SYMBOL(drm_hdcp_atomic_check);
Re: [Freedreno] [PATCH] drm/msm: potential error pointer dereference in init()
On 04/10/2021 13:38, Dan Carpenter wrote:
The msm_iommu_new() returns error pointers on failure so check for that
to avoid an Oops.
Fixes: ccac7ce373c1 ("drm/msm: Refactor address space initialization")
Signed-off-by: Dan Carpenter
---
Reviewed-by: Dmitry Baryshkov
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index ae48f41821cf..ad247c06e198 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -908,6 +908,10 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
return 0;
mmu = msm_iommu_new(dpu_kms->dev->dev, domain);
+ if (IS_ERR(mmu)) {
+ iommu_domain_free(domain);
+ return PTR_ERR(mmu);
+ }
aspace = msm_gem_address_space_create(mmu, "dpu1",
0x1000, 0x1 - 0x1000);
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH] drm/msm: potential error pointer dereference in init()
On 2021-10-04 03:38, Dan Carpenter wrote:
The msm_iommu_new() returns error pointers on failure so check for that
to avoid an Oops.
Fixes: ccac7ce373c1 ("drm/msm: Refactor address space initialization")
Signed-off-by: Dan Carpenter
Reviewed-by: Abhinav Kumar
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index ae48f41821cf..ad247c06e198 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -908,6 +908,10 @@ static int _dpu_kms_mmu_init(struct dpu_kms
*dpu_kms)
return 0;
mmu = msm_iommu_new(dpu_kms->dev->dev, domain);
+ if (IS_ERR(mmu)) {
+ iommu_domain_free(domain);
+ return PTR_ERR(mmu);
+ }
aspace = msm_gem_address_space_create(mmu, "dpu1",
0x1000, 0x1 - 0x1000);
Re: [Freedreno] [PATCH] drm/msm/disp: fix endian bug in debugfs code
On 2021-10-04 06:47, Dan Carpenter wrote:
The "vbif->features" is type unsigned long but the debugfs file
is treating it as a u32 type. This will work in little endian
systems, but the correct thing is to change the debugfs to use
an unsigned long.
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Dan Carpenter
Reviewed-by: Abhinav Kumar
---
You might wonder why this code has so many casts. It's required
because
this data is const. Which is fine because the file is read only.
drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
index 21d20373eb8b..e645a886e3c6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
@@ -305,8 +305,8 @@ void dpu_debugfs_vbif_init(struct dpu_kms
*dpu_kms, struct dentry *debugfs_root)
debugfs_vbif = debugfs_create_dir(vbif_name, entry);
- debugfs_create_u32("features", 0600, debugfs_vbif,
- (u32 *)>features);
+ debugfs_create_ulong("features", 0600, debugfs_vbif,
+(unsigned long *)>features);
debugfs_create_u32("xin_halt_timeout", 0400, debugfs_vbif,
(u32 *)>xin_halt_timeout);
Re: [Freedreno] [PATCH] drm/msm: Fix potential Oops in a6xx_gmu_rpmh_init()
On 04/10/2021 16:45, Dan Carpenter wrote:
There are two problems here:
1) The "seqptr" is used uninitalized when we free it at the end.
This looks like a nice catch, potentially causing troubles.
2) The a6xx_gmu_get_mmio() function returns error pointers. It never
returns true.
Fixes: 64245fc55172 ("drm/msm/a6xx: use AOP-initialized PDC for a650")
Fixes: f8fc924e088e ("drm/msm/a6xx: Fix PDC register overlap")
Signed-off-by: Dan Carpenter
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index a7c58018959f..3bd6e579ea89 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -512,11 +512,11 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
struct adreno_gpu *adreno_gpu = _gpu->base;
struct platform_device *pdev = to_platform_device(gmu->dev);
void __iomem *pdcptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc");
- void __iomem *seqptr;
+ void __iomem *seqptr = NULL;
uint32_t pdc_address_offset;
bool pdc_in_aop = false;
- if (!pdcptr)
+ if (IS_ERR(pdcptr))
goto err;
if (adreno_is_a650(adreno_gpu) || adreno_is_a660_family(adreno_gpu))
@@ -528,7 +528,7 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
if (!pdc_in_aop) {
seqptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc_seq");
- if (!seqptr)
+ if (IS_ERR(seqptr))
goto err;
}
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH] drm/msm/disp: fix endian bug in debugfs code
On 04/10/2021 16:47, Dan Carpenter wrote:
The "vbif->features" is type unsigned long but the debugfs file
is treating it as a u32 type. This will work in little endian
systems, but the correct thing is to change the debugfs to use
an unsigned long.
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Dan Carpenter
---
You might wonder why this code has so many casts. It's required because
this data is const. Which is fine because the file is read only.
drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
index 21d20373eb8b..e645a886e3c6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
@@ -305,8 +305,8 @@ void dpu_debugfs_vbif_init(struct dpu_kms *dpu_kms, struct
dentry *debugfs_root)
debugfs_vbif = debugfs_create_dir(vbif_name, entry);
- debugfs_create_u32("features", 0600, debugfs_vbif,
- (u32 *)>features);
+ debugfs_create_ulong("features", 0600, debugfs_vbif,
+(unsigned long *)>features);
As you are converting this to the ulong file, could you please also
remove the now-unnecessary type cast?
debugfs_create_u32("xin_halt_timeout", 0400, debugfs_vbif,
(u32 *)>xin_halt_timeout);
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v3 11/14] drm/msm/dp: Re-order dp_audio_put in deinit_sub_modules
On 01/10/2021 18:11, Sean Paul wrote:
From: Sean Paul
Audio is initialized last, it should be de-initialized first to match
the order in dp_init_sub_modules().
Reviewed-by: Abhinav Kumar
Reviewed-by: Stephen Boyd
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-12-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-12-s...@poorly.run
#v2
Reviewed-by: Dmitry Baryshkov
Changes in v2:
-None
---
drivers/gpu/drm/msm/dp/dp_display.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index fbe4c2cd52a3..19946024e235 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -714,9 +714,9 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp,
u32 data)
static void dp_display_deinit_sub_modules(struct dp_display_private *dp)
{
dp_debug_put(dp->debug);
+ dp_audio_put(dp->audio);
dp_panel_put(dp->panel);
dp_aux_put(dp->aux);
- dp_audio_put(dp->audio);
}
static int dp_init_sub_modules(struct dp_display_private *dp)
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v3 10/14] drm/msm/dpu: Remove encoder->enable() hack
On 01/10/2021 18:11, Sean Paul wrote:
From: Sean Paul
encoder->commit() was being misused because there were some global
resources which needed to be tweaked in encoder->enable() which were not
accessible in dpu_encoder.c. That is no longer true and the redirect
serves no purpose any longer. So remove the indirection.
Reviewed-by: Stephen Boyd
Tested-by: Stephen Boyd
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-11-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-11-s...@poorly.run
#v2
Reviewed-by: Dmitry Baryshkov
Changes in v2:
-None
Changes in v3:
-None
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 +
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 22 -
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 2 --
drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 4
4 files changed, 1 insertion(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 984f8a59cb73..ddc542a0d41f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2122,11 +2122,8 @@ static void dpu_encoder_frame_done_timeout(struct
timer_list *t)
static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
.mode_set = dpu_encoder_virt_mode_set,
.disable = dpu_encoder_virt_disable,
- .enable = dpu_kms_encoder_enable,
+ .enable = dpu_encoder_virt_enable,
.atomic_check = dpu_encoder_virt_atomic_check,
-
- /* This is called by dpu_kms_encoder_enable */
- .commit = dpu_encoder_virt_enable,
};
static const struct drm_encoder_funcs dpu_encoder_funcs = {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index fb0d9f781c66..4a0b55d145ad 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -381,28 +381,6 @@ static void dpu_kms_flush_commit(struct msm_kms *kms,
unsigned crtc_mask)
}
}
-/*
- * Override the encoder enable since we need to setup the inline rotator and do
- * some crtc magic before enabling any bridge that might be present.
- */
-void dpu_kms_encoder_enable(struct drm_encoder *encoder)
-{
- const struct drm_encoder_helper_funcs *funcs = encoder->helper_private;
- struct drm_device *dev = encoder->dev;
- struct drm_crtc *crtc;
-
- /* Forward this enable call to the commit hook */
- if (funcs && funcs->commit)
- funcs->commit(encoder);
-
- drm_for_each_crtc(crtc, dev) {
- if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder)))
- continue;
-
- trace_dpu_kms_enc_enable(DRMID(crtc));
- }
-}
-
static void dpu_kms_complete_commit(struct msm_kms *kms, unsigned crtc_mask)
{
struct dpu_kms *dpu_kms = to_dpu_kms(kms);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 323a6bce9e64..f1ebb60dacab 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -248,8 +248,6 @@ void *dpu_debugfs_get_root(struct dpu_kms *dpu_kms);
int dpu_enable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
void dpu_disable_vblank(struct msm_kms *kms, struct drm_crtc *crtc);
-void dpu_kms_encoder_enable(struct drm_encoder *encoder);
-
/**
* dpu_kms_get_clk_rate() - get the clock rate
* @dpu_kms: pointer to dpu_kms structure
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
index 37bba57675a8..54d74341e690 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
@@ -266,10 +266,6 @@ DEFINE_EVENT(dpu_drm_obj_template,
dpu_crtc_complete_commit,
TP_PROTO(uint32_t drm_id),
TP_ARGS(drm_id)
);
-DEFINE_EVENT(dpu_drm_obj_template, dpu_kms_enc_enable,
- TP_PROTO(uint32_t drm_id),
- TP_ARGS(drm_id)
-);
DEFINE_EVENT(dpu_drm_obj_template, dpu_kms_commit,
TP_PROTO(uint32_t drm_id),
TP_ARGS(drm_id)
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v3 09/14] drm/msm/dpu: Remove useless checks in dpu_encoder
On 01/10/2021 18:11, Sean Paul wrote:
From: Sean Paul
A couple more useless checks to remove in dpu_encoder.
Reviewed-by: Stephen Boyd
Signed-off-by: Sean Paul
Link:
https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-10-s...@poorly.run
#v1
Link:
https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-10-s...@poorly.run
#v2
Reviewed-by: Dmitry Baryshkov
Changes in v2:
-None
Changes in v3:
-None
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12
1 file changed, 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 0e9d3fa1544b..984f8a59cb73 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1153,10 +1153,6 @@ static void dpu_encoder_virt_enable(struct drm_encoder
*drm_enc)
struct msm_drm_private *priv;
struct drm_display_mode *cur_mode = NULL;
- if (!drm_enc) {
- DPU_ERROR("invalid encoder\n");
- return;
- }
dpu_enc = to_dpu_encoder_virt(drm_enc);
mutex_lock(_enc->enc_lock);
@@ -1203,14 +1199,6 @@ static void dpu_encoder_virt_disable(struct drm_encoder
*drm_enc)
struct msm_drm_private *priv;
int i = 0;
- if (!drm_enc) {
- DPU_ERROR("invalid encoder\n");
- return;
- } else if (!drm_enc->dev) {
- DPU_ERROR("invalid dev\n");
- return;
- }
-
dpu_enc = to_dpu_encoder_virt(drm_enc);
DPU_DEBUG_ENC(dpu_enc, "\n");
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH v3 08/14] drm/msm/dpu_kms: Re-order dpu includes
On 01/10/2021 18:11, Sean Paul wrote: From: Sean Paul Make includes alphabetical in dpu_kms.c Reviewed-by: Abhinav Kumar Reviewed-by: Stephen Boyd Signed-off-by: Sean Paul Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-9-s...@poorly.run #v1 Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-9-s...@poorly.run #v2 Changes in v2: -None Changes in v3: -None Reviewed-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index ae48f41821cf..fb0d9f781c66 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -21,14 +21,14 @@ #include "msm_gem.h" #include "disp/msm_disp_snapshot.h" -#include "dpu_kms.h" #include "dpu_core_irq.h" +#include "dpu_crtc.h" +#include "dpu_encoder.h" #include "dpu_formats.h" #include "dpu_hw_vbif.h" -#include "dpu_vbif.h" -#include "dpu_encoder.h" +#include "dpu_kms.h" #include "dpu_plane.h" -#include "dpu_crtc.h" +#include "dpu_vbif.h" #define CREATE_TRACE_POINTS #include "dpu_trace.h" -- With best wishes Dmitry
Re: [Freedreno] [PATCH v3 12/14] dt-bindings: msm/dp: Add bindings for HDCP registers
On Fri 01 Oct 10:11 CDT 2021, Sean Paul wrote:
> From: Sean Paul
>
> This patch adds the bindings for the MSM DisplayPort HDCP registers
> which are required to write the HDCP key into the display controller as
> well as the registers to enable HDCP authentication/key
> exchange/encryption.
>
> We'll use a new compatible string for this since the fields are optional.
>
> Cc: Rob Herring
> Cc: Stephen Boyd
> Signed-off-by: Sean Paul
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-13-s...@poorly.run
> #v1
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-13-s...@poorly.run
> #v2
>
> Changes in v2:
> -Drop register range names (Stephen)
> -Fix yaml errors (Rob)
> Changes in v3:
> -Add new compatible string for dp-hdcp
> -Add descriptions to reg
> -Add minItems/maxItems to reg
> -Make reg depend on the new hdcp compatible string
> ---
>
> Disclaimer: I really don't know if this is the right way to approach
> this. I tried using examples from other bindings, but feedback would be
> very much welcome on how I could add the optional register ranges.
>
>
> .../bindings/display/msm/dp-controller.yaml | 34 ---
> 1 file changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index 64d8d9e5e47a..a176f97b2f4c 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -17,9 +17,10 @@ properties:
>compatible:
> enum:
>- qcom,sc7180-dp
> + - qcom,sc7180-dp-hdcp
>
> - reg:
> -maxItems: 1
> + # See compatible-specific constraints below.
> + reg: true
>
>interrupts:
> maxItems: 1
> @@ -89,6 +90,29 @@ required:
>- power-domains
>- ports
>
> +allOf:
> + - if:
> + properties:
> +compatible:
> + contains:
> +const: qcom,sc7180-dp-hdcp
> +then:
> + properties:
> +reg:
> + minItems: 3
> + maxItems: 3
> + items:
> +- description: Registers for base DP functionality
> +- description: (Optional) Registers for HDCP device key injection
> +- description: (Optional) Registers for HDCP TrustZone
> interaction
> +else:
> + properties:
> +reg:
> + minItems: 1
> + maxItems: 1
> + items:
> +- description: Registers for base DP functionality
> +
> additionalProperties: false
>
> examples:
> @@ -99,8 +123,10 @@ examples:
> #include
>
> displayport-controller@ae9 {
> -compatible = "qcom,sc7180-dp";
> -reg = <0xae9 0x1400>;
> +compatible = "qcom,sc7180-dp-hdcp";
> +reg = <0 0x0ae9 0 0x1400>,
> + <0 0x0aed1000 0 0x174>,
> + <0 0x0aee1000 0 0x2c>;
Forgot to mention, #address-cells = #size-cells = <1> in the example
"environment", so you have to omit the lone 0s in the example to make it
pass the tests.
Regards,
Bjorn
> interrupt-parent = <>;
> interrupts = <12>;
> clocks = < DISP_CC_MDSS_AHB_CLK>,
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>
Re: [Freedreno] [PATCH v3 12/14] dt-bindings: msm/dp: Add bindings for HDCP registers
On Fri 01 Oct 10:11 CDT 2021, Sean Paul wrote:
> From: Sean Paul
>
> This patch adds the bindings for the MSM DisplayPort HDCP registers
> which are required to write the HDCP key into the display controller as
> well as the registers to enable HDCP authentication/key
> exchange/encryption.
>
> We'll use a new compatible string for this since the fields are optional.
>
I don't think you need a new compatible, in particular since I presume
we should use the hdcp compatible in all platforms? Or is there a reason
for not picking that one?
Instead I suggest that you simply do minItems: 1, maxItems: 3 and detect
which of the two cases you have in the driver.
PS. I hope to get
https://lore.kernel.org/linux-arm-msm/20211001174400.981707-1-bjorn.anders...@linaro.org/
landed before we add these new optional regions...
Regards,
Bjorn
> Cc: Rob Herring
> Cc: Stephen Boyd
> Signed-off-by: Sean Paul
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-13-s...@poorly.run
> #v1
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-13-s...@poorly.run
> #v2
>
> Changes in v2:
> -Drop register range names (Stephen)
> -Fix yaml errors (Rob)
> Changes in v3:
> -Add new compatible string for dp-hdcp
> -Add descriptions to reg
> -Add minItems/maxItems to reg
> -Make reg depend on the new hdcp compatible string
> ---
>
> Disclaimer: I really don't know if this is the right way to approach
> this. I tried using examples from other bindings, but feedback would be
> very much welcome on how I could add the optional register ranges.
>
>
> .../bindings/display/msm/dp-controller.yaml | 34 ---
> 1 file changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> index 64d8d9e5e47a..a176f97b2f4c 100644
> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
> @@ -17,9 +17,10 @@ properties:
>compatible:
> enum:
>- qcom,sc7180-dp
> + - qcom,sc7180-dp-hdcp
>
> - reg:
> -maxItems: 1
> + # See compatible-specific constraints below.
> + reg: true
>
>interrupts:
> maxItems: 1
> @@ -89,6 +90,29 @@ required:
>- power-domains
>- ports
>
> +allOf:
> + - if:
> + properties:
> +compatible:
> + contains:
> +const: qcom,sc7180-dp-hdcp
> +then:
> + properties:
> +reg:
> + minItems: 3
> + maxItems: 3
> + items:
> +- description: Registers for base DP functionality
> +- description: (Optional) Registers for HDCP device key injection
> +- description: (Optional) Registers for HDCP TrustZone
> interaction
> +else:
> + properties:
> +reg:
> + minItems: 1
> + maxItems: 1
> + items:
> +- description: Registers for base DP functionality
> +
> additionalProperties: false
>
> examples:
> @@ -99,8 +123,10 @@ examples:
> #include
>
> displayport-controller@ae9 {
> -compatible = "qcom,sc7180-dp";
> -reg = <0xae9 0x1400>;
> +compatible = "qcom,sc7180-dp-hdcp";
> +reg = <0 0x0ae9 0 0x1400>,
> + <0 0x0aed1000 0 0x174>,
> + <0 0x0aee1000 0 0x2c>;
> interrupt-parent = <>;
> interrupts = <12>;
> clocks = < DISP_CC_MDSS_AHB_CLK>,
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>
Re: [Freedreno] [PATCH v3 1/3] dt-bindings: msm: dsi: Add MSM8953 dsi phy
On Tue, 28 Sep 2021 18:49:27 +0530, Sireesh Kodali wrote: > SoCs based on the MSM8953 platform use the 14nm DSI PHY driver > > Signed-off-by: Sireesh Kodali > --- > Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring
Re: [Freedreno] [PATCH] drm/msm/a6xx: Serialize GMU communication
On 10/2/2021 1:02 AM, Rob Clark wrote:
From: Rob Clark
I've seen some crashes in our crash reporting that *look* like multiple
threads stomping on each other while communicating with GMU. So wrap
all those paths in a lock.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6
drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 3 ++
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 40 +++
3 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index a7c58018959f..8b73f70766a4 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -296,6 +296,8 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state)
u32 val;
int request, ack;
+ WARN_ON_ONCE(!mutex_is_locked(>lock));
+
if (state >= ARRAY_SIZE(a6xx_gmu_oob_bits))
return -EINVAL;
@@ -337,6 +339,8 @@ void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state)
{
int bit;
+ WARN_ON_ONCE(!mutex_is_locked(>lock));
+
if (state >= ARRAY_SIZE(a6xx_gmu_oob_bits))
return;
@@ -1482,6 +1486,8 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
if (!pdev)
return -ENODEV;
+ mutex_init(>lock);
+
gmu->dev = >dev;
of_dma_configure(gmu->dev, node, true);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index 3c74f64e3126..84bd516f01e8 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -44,6 +44,9 @@ struct a6xx_gmu_bo {
struct a6xx_gmu {
struct device *dev;
+ /* For serializing communication with the GMU: */
+ struct mutex lock;
+
struct msm_gem_address_space *aspace;
void * __iomem mmio;
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index f6a4dbef796b..bd7bdeff5d6f 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -881,7 +881,7 @@ static int a6xx_zap_shader_init(struct msm_gpu *gpu)
A6XX_RBBM_INT_0_MASK_UCHE_OOB_ACCESS | \
A6XX_RBBM_INT_0_MASK_UCHE_TRAP_INTR)
-static int a6xx_hw_init(struct msm_gpu *gpu)
+static int hw_init(struct msm_gpu *gpu)
{
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
@@ -1135,6 +1135,19 @@ static int a6xx_hw_init(struct msm_gpu *gpu)
return ret;
}
+static int a6xx_hw_init(struct msm_gpu *gpu)
+{
+ struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+ struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
+ int ret;
+
+ mutex_lock(_gpu->gmu.lock);
+ ret = hw_init(gpu);
+ mutex_unlock(_gpu->gmu.lock);
+
+ return ret;
+}
+
static void a6xx_dump(struct msm_gpu *gpu)
{
DRM_DEV_INFO(>pdev->dev, "status: %08x\n",
@@ -1509,7 +1522,9 @@ static int a6xx_pm_resume(struct msm_gpu *gpu)
trace_msm_gpu_resume(0);
+ mutex_lock(_gpu->gmu.lock);
ret = a6xx_gmu_resume(a6xx_gpu);
+ mutex_unlock(_gpu->gmu.lock);
if (ret)
return ret;
@@ -1532,7 +1547,9 @@ static int a6xx_pm_suspend(struct msm_gpu *gpu)
msm_devfreq_suspend(gpu);
+ mutex_lock(_gpu->gmu.lock);
ret = a6xx_gmu_stop(a6xx_gpu);
+ mutex_unlock(_gpu->gmu.lock);
if (ret)
return ret;
@@ -1547,18 +1564,19 @@ static int a6xx_get_timestamp(struct msm_gpu *gpu, uint64_t *value)
{
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
- static DEFINE_MUTEX(perfcounter_oob);
- mutex_lock(_oob);
+ mutex_lock(_gpu->gmu.lock);
/* Force the GPU power on so we can read this register */
a6xx_gmu_set_oob(_gpu->gmu, GMU_OOB_PERFCOUNTER_SET);
*value = gpu_read64(gpu, REG_A6XX_CP_ALWAYS_ON_COUNTER_LO,
- REG_A6XX_CP_ALWAYS_ON_COUNTER_HI);
+ REG_A6XX_CP_ALWAYS_ON_COUNTER_HI);
a6xx_gmu_clear_oob(_gpu->gmu, GMU_OOB_PERFCOUNTER_SET);
- mutex_unlock(_oob);
+
+ mutex_unlock(_gpu->gmu.lock);
+
return 0;
}
@@ -1622,6 +1640,16 @@ static unsigned long a6xx_gpu_busy(struct msm_gpu *gpu)
return (unsigned long)busy_time;
}
+void a6xx_gpu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp)
+{
+ struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
+ struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
+
+ mutex_lock(_gpu->gmu.lock);
+ a6xx_gmu_set_freq(gpu, opp);
+ mutex_unlock(_gpu->gmu.lock);
+}
+
static struct msm_gem_address_space *
a6xx_create_address_space(struct msm_gpu *gpu, struct platform_device *pdev)
{
@@ -1766,7 +1794,7 @@ static const struct adreno_gpu_funcs funcs = {
Re: [Freedreno] [PATCH] drm/msm/dsi: use bulk clk API
On Fri 01 Oct 18:27 PDT 2021, Dmitry Baryshkov wrote:
> Use clk_bulk_* API instead of hand-coding them. Note, this drops support
> for legacy clk naming (e.g. "iface_clk" instead of just "iface"),
> however all in-kernel device trees were converted long long ago. The
> warning is present there since 2017.
>
Nice!
Reviewed-by: Bjorn Andersson
Regards,
Bjorn
> Signed-off-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/msm/dsi/dsi_host.c | 59 ++
> 1 file changed, 12 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index e269df285136..3b81f40bba2e 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -106,7 +106,8 @@ struct msm_dsi_host {
> phys_addr_t ctrl_size;
> struct regulator_bulk_data supplies[DSI_DEV_REGULATOR_MAX];
>
> - struct clk *bus_clks[DSI_BUS_CLK_MAX];
> + int num_bus_clks;
> + struct clk_bulk_data bus_clks[DSI_BUS_CLK_MAX];
>
> struct clk *byte_clk;
> struct clk *esc_clk;
> @@ -374,15 +375,14 @@ static int dsi_clk_init(struct msm_dsi_host *msm_host)
> int i, ret = 0;
>
> /* get bus clocks */
> - for (i = 0; i < cfg->num_bus_clks; i++) {
> - msm_host->bus_clks[i] = msm_clk_get(pdev,
> - cfg->bus_clk_names[i]);
> - if (IS_ERR(msm_host->bus_clks[i])) {
> - ret = PTR_ERR(msm_host->bus_clks[i]);
> - pr_err("%s: Unable to get %s clock, ret = %d\n",
> - __func__, cfg->bus_clk_names[i], ret);
> - goto exit;
> - }
> + for (i = 0; i < cfg->num_bus_clks; i++)
> + msm_host->bus_clks[i].id = cfg->bus_clk_names[i];
> + msm_host->num_bus_clks = cfg->num_bus_clks;
> +
> + ret = devm_clk_bulk_get(>dev, msm_host->num_bus_clks,
> msm_host->bus_clks);
> + if (ret < 0) {
> + dev_err(>dev, "Unable to get clocks, ret = %d\n", ret);
> + goto exit;
> }
>
> /* get link and source clocks */
> @@ -433,41 +433,6 @@ static int dsi_clk_init(struct msm_dsi_host *msm_host)
> return ret;
> }
>
> -static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)
> -{
> - const struct msm_dsi_config *cfg = msm_host->cfg_hnd->cfg;
> - int i, ret;
> -
> - DBG("id=%d", msm_host->id);
> -
> - for (i = 0; i < cfg->num_bus_clks; i++) {
> - ret = clk_prepare_enable(msm_host->bus_clks[i]);
> - if (ret) {
> - pr_err("%s: failed to enable bus clock %d ret %d\n",
> - __func__, i, ret);
> - goto err;
> - }
> - }
> -
> - return 0;
> -err:
> - for (; i > 0; i--)
> - clk_disable_unprepare(msm_host->bus_clks[i]);
> -
> - return ret;
> -}
> -
> -static void dsi_bus_clk_disable(struct msm_dsi_host *msm_host)
> -{
> - const struct msm_dsi_config *cfg = msm_host->cfg_hnd->cfg;
> - int i;
> -
> - DBG("");
> -
> - for (i = cfg->num_bus_clks - 1; i >= 0; i--)
> - clk_disable_unprepare(msm_host->bus_clks[i]);
> -}
> -
> int msm_dsi_runtime_suspend(struct device *dev)
> {
> struct platform_device *pdev = to_platform_device(dev);
> @@ -478,7 +443,7 @@ int msm_dsi_runtime_suspend(struct device *dev)
> if (!msm_host->cfg_hnd)
> return 0;
>
> - dsi_bus_clk_disable(msm_host);
> + clk_bulk_disable_unprepare(msm_host->num_bus_clks, msm_host->bus_clks);
>
> return 0;
> }
> @@ -493,7 +458,7 @@ int msm_dsi_runtime_resume(struct device *dev)
> if (!msm_host->cfg_hnd)
> return 0;
>
> - return dsi_bus_clk_enable(msm_host);
> + return clk_bulk_prepare_enable(msm_host->num_bus_clks,
> msm_host->bus_clks);
> }
>
> int dsi_link_clk_set_rate_6g(struct msm_dsi_host *msm_host)
> --
> 2.33.0
>
[Freedreno] [PATCH] drm/msm: Fix potential Oops in a6xx_gmu_rpmh_init()
There are two problems here:
1) The "seqptr" is used uninitalized when we free it at the end.
2) The a6xx_gmu_get_mmio() function returns error pointers. It never
returns true.
Fixes: 64245fc55172 ("drm/msm/a6xx: use AOP-initialized PDC for a650")
Fixes: f8fc924e088e ("drm/msm/a6xx: Fix PDC register overlap")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index a7c58018959f..3bd6e579ea89 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -512,11 +512,11 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
struct adreno_gpu *adreno_gpu = _gpu->base;
struct platform_device *pdev = to_platform_device(gmu->dev);
void __iomem *pdcptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc");
- void __iomem *seqptr;
+ void __iomem *seqptr = NULL;
uint32_t pdc_address_offset;
bool pdc_in_aop = false;
- if (!pdcptr)
+ if (IS_ERR(pdcptr))
goto err;
if (adreno_is_a650(adreno_gpu) || adreno_is_a660_family(adreno_gpu))
@@ -528,7 +528,7 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
if (!pdc_in_aop) {
seqptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc_seq");
- if (!seqptr)
+ if (IS_ERR(seqptr))
goto err;
}
--
2.20.1
[Freedreno] [PATCH] drm/msm/disp: fix endian bug in debugfs code
The "vbif->features" is type unsigned long but the debugfs file
is treating it as a u32 type. This will work in little endian
systems, but the correct thing is to change the debugfs to use
an unsigned long.
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Dan Carpenter
---
You might wonder why this code has so many casts. It's required because
this data is const. Which is fine because the file is read only.
drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
index 21d20373eb8b..e645a886e3c6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
@@ -305,8 +305,8 @@ void dpu_debugfs_vbif_init(struct dpu_kms *dpu_kms, struct
dentry *debugfs_root)
debugfs_vbif = debugfs_create_dir(vbif_name, entry);
- debugfs_create_u32("features", 0600, debugfs_vbif,
- (u32 *)>features);
+ debugfs_create_ulong("features", 0600, debugfs_vbif,
+(unsigned long *)>features);
debugfs_create_u32("xin_halt_timeout", 0400, debugfs_vbif,
(u32 *)>xin_halt_timeout);
--
2.20.1
[Freedreno] [bug report] drm/msm: Add SDM845 DPU support
Hello MSM Devs,
The patch 25fdd5933e4c: "drm/msm: Add SDM845 DPU support" from Jun
27, 2018, leads to the following Smatch static checker warnings:
drivers/gpu/drm/msm/msm_gpu.c:301 msm_gpu_crashstate_capture() error: potential
null dereference 'state->bos'. (kcalloc returns null)
drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c:177
msm_disp_snapshot_add_block() error: potential null dereference 'new_blk'.
(kzalloc returns null)
drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c:96 mdp5_plane_reset() error:
potential null dereference 'mdp5_state'. (kzalloc returns null)
drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c:98 mdp5_plane_reset() error:
potential null dereference 'mdp5_state'. (kzalloc returns null)
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:963 dpu_crtc_atomic_check() error:
potential null dereference 'pstates'. (kzalloc returns null)
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:1009 dpu_crtc_atomic_check() error:
potential null dereference 'pstates'. (kzalloc returns null)
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:1075 dpu_crtc_atomic_check() error:
potential null dereference 'pstates'. (kzalloc returns null)
drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c:214 dpu_core_irq_preinstall()
error: potential null dereference 'dpu_kms->irq_obj.irq_cb_tbl'. (kcalloc
returns null)
drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c:215 dpu_core_irq_preinstall()
error: potential null dereference 'dpu_kms->irq_obj.irq_counts'. (kcalloc
returns null)
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
901 static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
902 struct drm_atomic_state *state)
903 {
904 struct drm_crtc_state *crtc_state =
drm_atomic_get_new_crtc_state(state,
905
crtc);
906 struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
907 struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state);
908 struct plane_state *pstates;
909
910 const struct drm_plane_state *pstate;
911 struct drm_plane *plane;
912 struct drm_display_mode *mode;
913
914 int cnt = 0, rc = 0, mixer_width = 0, i, z_pos;
915
916 struct dpu_multirect_plane_states multirect_plane[DPU_STAGE_MAX
* 2];
917 int multirect_count = 0;
918 const struct drm_plane_state *pipe_staged[SSPP_MAX];
919 int left_zpos_cnt = 0, right_zpos_cnt = 0;
920 struct drm_rect crtc_rect = { 0 };
921
922 pstates = kzalloc(sizeof(*pstates) * DPU_STAGE_MAX * 4,
GFP_KERNEL);
^
There are a bunch of allocations with no checks for NULL
923
924 if (!crtc_state->enable || !crtc_state->active) {
925 DRM_DEBUG_ATOMIC("crtc%d -> enable %d, active %d, skip
atomic_check\n",
926 crtc->base.id, crtc_state->enable,
927 crtc_state->active);
928 memset(>new_perf, 0, sizeof(cstate->new_perf));
929 goto end;
930 }
931
932 mode = _state->adjusted_mode;
933 DRM_DEBUG_ATOMIC("%s: check\n", dpu_crtc->name);
934
935 /* force a full mode set if active state changed */
936 if (crtc_state->active_changed)
937 crtc_state->mode_changed = true;
938
939 memset(pipe_staged, 0, sizeof(pipe_staged));
940
941 if (cstate->num_mixers) {
942 mixer_width = mode->hdisplay / cstate->num_mixers;
943
944 _dpu_crtc_setup_lm_bounds(crtc, crtc_state);
945 }
946
947 crtc_rect.x2 = mode->hdisplay;
948 crtc_rect.y2 = mode->vdisplay;
949
950 /* get plane state for all drm planes associated with crtc
state */
951 drm_atomic_crtc_state_for_each_plane_state(plane, pstate,
crtc_state) {
952 struct drm_rect dst, clip = crtc_rect;
953
954 if (IS_ERR_OR_NULL(pstate)) {
955 rc = PTR_ERR(pstate);
956 DPU_ERROR("%s: failed to get plane%d state,
%d\n",
957 dpu_crtc->name, plane->base.id,
rc);
958 goto end;
959 }
960 if (cnt >= DPU_STAGE_MAX * 4)
961 continue;
962
--> 963 pstates[cnt].dpu_pstate = to_dpu_plane_state(pstate);
964 pstates[cnt].drm_pstate = pstate;
965 pstates[cnt].stage = pstate->normalized_zpos;
966 pstates[cnt].pipe_id = dpu_plane_pipe(plane);
967
regards,
dan carpenter
[Freedreno] [PATCH] drm/msm: potential error pointer dereference in init()
The msm_iommu_new() returns error pointers on failure so check for that
to avoid an Oops.
Fixes: ccac7ce373c1 ("drm/msm: Refactor address space initialization")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index ae48f41821cf..ad247c06e198 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -908,6 +908,10 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
return 0;
mmu = msm_iommu_new(dpu_kms->dev->dev, domain);
+ if (IS_ERR(mmu)) {
+ iommu_domain_free(domain);
+ return PTR_ERR(mmu);
+ }
aspace = msm_gem_address_space_create(mmu, "dpu1",
0x1000, 0x1 - 0x1000);
--
2.20.1
Re: [Freedreno] [Intel-gfx] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Mon, 04 Oct 2021, Ville Syrjälä wrote: > On Sun, Oct 03, 2021 at 12:32:14AM +0200, Fernando Ramos wrote: >> On 21/10/02 09:13AM, Fernando Ramos wrote: >> > >> > Sean, could you revert the whole patch series? I'll have a deeper look >> > into the >> > patch set and come up with a v3 where all these issues will be addressed. >> > >> >> Hi Sean, >> >> I now understand the nature of the issue that caused the problem with i915 >> and >> have proceed to remove the global context structure (which revealed a similar >> issue in the amdgpu driver). >> >> I have prepared a V3 version of the patch set where these issues should >> hopefully be fixed for both the i915 and amdgpu drivers. >> >> In order to prevent causing more disruption, could you tell me what the >> proper >> way to proceed would be? In particular: >> >> 1. Is there any place where I can push my changes so that they are tested >> on a i915 machine? (Some type of automated pool) > > cc:intel-gfx, which it looks like you did, _but_ your patches did > did not even apply against drm-tip so our CI rejected it. There was > a reply to the patches from CI indicating that. And that is one > reason I probably just ignored the whole thing. If it doesn't > even apply/build it's not worth my time to read. > >> >> 2. I can test the amdgpu driver on my machine but, what about all the other >> architectures? What is the standard procedure? Should I simply publish >> V3 >> and wait for feedback from the different vendors? (I would hate to >> cause a >> simular situation again) >> >> 3. Should I post V3 on top of drm-next or drm-misc-next? > > The normal rule is: always work on drm-tip. That is what gets > tested by our CI as well. Yes, it does mean a bit of extra hurdles > during development since drm-tip is a rebasing tree, but there are > tools like dim retip to help out here. > > As for where to merge them. I would generally recommed against merging > i915 patches through drm-misc unless there is a very compelling reason > to do so. i915 is a fast moving target and if there are significant > changes coming in via drm-misc they usually will cause conflicts for > people during drm-tip rebuild. Also I would expect to see an ack > requested from i915 maintainers for merging anything significant via > drm-misc, which I don't think happened in this case. Indeed. All other things aside, it looks like it has enough conflict potential to warrant merging via drm-intel anyway. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [Intel-gfx] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Sun, Oct 03, 2021 at 12:32:14AM +0200, Fernando Ramos wrote: > On 21/10/02 09:13AM, Fernando Ramos wrote: > > > > Sean, could you revert the whole patch series? I'll have a deeper look into > > the > > patch set and come up with a v3 where all these issues will be addressed. > > > > Hi Sean, > > I now understand the nature of the issue that caused the problem with i915 and > have proceed to remove the global context structure (which revealed a similar > issue in the amdgpu driver). > > I have prepared a V3 version of the patch set where these issues should > hopefully be fixed for both the i915 and amdgpu drivers. > > In order to prevent causing more disruption, could you tell me what the proper > way to proceed would be? In particular: > > 1. Is there any place where I can push my changes so that they are tested > on a i915 machine? (Some type of automated pool) cc:intel-gfx, which it looks like you did, _but_ your patches did did not even apply against drm-tip so our CI rejected it. There was a reply to the patches from CI indicating that. And that is one reason I probably just ignored the whole thing. If it doesn't even apply/build it's not worth my time to read. > > 2. I can test the amdgpu driver on my machine but, what about all the other > architectures? What is the standard procedure? Should I simply publish V3 > and wait for feedback from the different vendors? (I would hate to cause > a > simular situation again) > > 3. Should I post V3 on top of drm-next or drm-misc-next? The normal rule is: always work on drm-tip. That is what gets tested by our CI as well. Yes, it does mean a bit of extra hurdles during development since drm-tip is a rebasing tree, but there are tools like dim retip to help out here. As for where to merge them. I would generally recommed against merging i915 patches through drm-misc unless there is a very compelling reason to do so. i915 is a fast moving target and if there are significant changes coming in via drm-misc they usually will cause conflicts for people during drm-tip rebuild. Also I would expect to see an ack requested from i915 maintainers for merging anything significant via drm-misc, which I don't think happened in this case. -- Ville Syrjälä Intel
Re: [Freedreno] [Intel-gfx] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Sat, Oct 02, 2021 at 07:28:02PM +0200, Fernando Ramos wrote:
> On 21/10/02 09:13AM, Fernando Ramos wrote:
> > On 21/10/02 05:30AM, Ville Syrjälä wrote:
> > > On Sat, Oct 02, 2021 at 01:05:47AM +0300, Ville Syrjälä wrote:
> > > > On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote:
> > > > > On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote:
> > > > > > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote:
> > > > > > >
> > > > > > > Thank you for revising, Fernando! I've pushed the set to
> > > > > > > drm-misc-next (along
> > > > > > > with the necessary drm-tip conflict resolutions).
> > > > > >
> > > > > > Ugh. Did anyone actually review the locking changes this does?
> > > > > > I shot the previous i915 stuff down because the commit messages
> > > > > > did not address any of it.
> > > > >
> > > > > I reviewed the set on 9/17, I didn't see your feedback on that thread.
> > > >
> > > > It was much earlir than that.
> > > > https://lists.freedesktop.org/archives/dri-devel/2021-June/313193.html
>
> Sorry, I'm new to this and it did not occur to me to search for similar
> patches
> in the mailing list archives in case there were additional comments that
> applied
> to my change set.
>
> In case I had done that I would have found that, as you mentioned, you had
> already raised two issues back in June:
>
> On Tue, Jun 29, 2021, Ville Syrjälä wrote:
> >
> > That looks wrong. You're using a private ctx here, but still
> > passing dev->mode_config.acquire_ctx to the lower level stuff.
> >
> > Also DRM_MODESET_LOCK_ALL_{BEGIN,END}() do not seem to be
> > equivalent to drm_modeset_{lock,unlock}_all() when it comes to
> > mode_config.mutex. So would need a proper review whether we
> > actually need that lock or not.
>
> The first one was pointing out the same error I would later repeat in my patch
> series (ups).
>
> After further inspection of the code it looks to me that changing this:
>
> intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx);
>
> ...into this:
>
> intel_modeset_setup_hw_state(dev, );
>
> ...would be enough.
Yes.
>
> Why? The only difference between the old drm_modeset_{lock,unlock}_all()
> functions and the new DRM_MODESET_LOCK_ALL_{BEGIN,END}() macros is that the
> former use a global context stored in dev->mode_config.acquire_ctx while the
> latter depend on a user provided one (typically in the stack).
>
> In the old (working) code the global context structure is freed in
> drm_modeset_unlock_all() thus we are sure no one is holding a reference to it
> at
> that point. This means that as long as no one accesses the global
> dev->mode_config.acquire_ctx context in the block that runs between lock/BEGIN
> and unlock/END, the code should be equivalent before and after my changes.
>
> In fact, now that my patch series removes the drm_modeset_{lock,unlock}_all()
> functions, the acquire_ctx field of the drm_mode_config structure should be
> deleted:
>
> /**
> * @acquire_ctx:
> *
> * Global implicit acquire context used by atomic drivers for legacy
> * IOCTLs. Deprecated, since implicit locking contexts make it
> * impossible to use driver-private drm_modeset_lock. Users of
> * this must hold @mutex.
> */
> struct drm_modeset_acquire_ctx *acquire_ctx;
>
> If I had done that (ie. removing this field) I would have detected the problem
> when compiling.
>
> There is another place (in the amdgpu driver) where this field is still being
> referenced, but before I investigate that I would like to know if you agree
> that
> this is a good path to follow.
Yeah, removing the mode_config.acquire_ctx is a good idea if it's
no longer needed.
>
> Regarding the second issue you raised...
>
> > Also DRM_MODESET_LOCK_ALL_{BEGIN,END}() do not seem to be
> > equivalent to drm_modeset_{lock,unlock}_all() when it comes to
> > mode_config.mutex. So would need a proper review whether we
> > actually need that lock or not.
>
> ...the only difference regarding mode_config.mutex I see is that in the new
> macros the mutex is locked only under this condition:
>
> if (!drm_drv_uses_atomic_modeset(dev))
>
> ...which seems reasonable, right? Is this what you were referring to or is it
> something else?
In order to eliminate the lock one first has to determine what that lock
might be protecting here, and then prove that such protection is not
actually needed.
--
Ville Syrjälä
Intel
Re: [Freedreno] [PATCH 3/3] drm/msm/dsi: fix signedness bug in msm_dsi_host_cmd_rx()
On Sat, Oct 02, 2021 at 01:59:56AM +0300, Dmitry Baryshkov wrote:
> On 01/10/2021 15:36, Dan Carpenter wrote:
> > The "msg->tx_len" variable is type size_t so if dsi_cmds2buf_tx()
> > returns a negative error code that it type promoted to a high positive
> > value and treat as a success. The second problem with this code is
> > that it can return meaningless positive values on error.
>
> It looks to me that this piece of code is not fully correct at all.
> dsi_cmds2bus_tx would return the size of DSI packet, not the size of the DSI
> buffer.
Ugh... I misread what you were saying. I was thinking I could just
check for negatives. This sounds like struct_size() thing?
>
> Could you please be more specific, which 'meaningless positive values' were
> you receiving?
>
Returning any positive values at this point is a bug. It's supposed to
return the number of bytes that were recieved.
And there is another bug as well:
drivers/gpu/drm/msm/dsi/dsi_host.c
1370 static int dsi_cmds2buf_tx(struct msm_dsi_host *msm_host,
1371 const struct mipi_dsi_msg *msg)
1372 {
1373 int len, ret;
1374 int bllp_len = msm_host->mode->hdisplay *
1375 dsi_get_bpp(msm_host->format) / 8;
1376
1377 len = dsi_cmd_dma_add(msm_host, msg);
1378 if (!len) {
The dsi_cmd_dma_add() returns negative error codes so this check should
be "if (len <= 0) {".
1379 pr_err("%s: failed to add cmd type = 0x%x\n",
1380 __func__, msg->type);
1381 return -EINVAL;
1382 }
1383
I'm not sure about the size of "the DSI packet" Could you handle this
one and give me a Reported-by tag? That's probably simpler than another
back and forth on email.
regards,
dan carpenter
Re: [Freedreno] [PATCH 3/3] drm/msm/dsi: fix signedness bug in msm_dsi_host_cmd_rx()
On Sat, Oct 02, 2021 at 01:59:56AM +0300, Dmitry Baryshkov wrote: > On 01/10/2021 15:36, Dan Carpenter wrote: > > The "msg->tx_len" variable is type size_t so if dsi_cmds2buf_tx() > > returns a negative error code that it type promoted to a high positive > > value and treat as a success. The second problem with this code is > > that it can return meaningless positive values on error. > > It looks to me that this piece of code is not fully correct at all. > dsi_cmds2bus_tx would return the size of DSI packet, not the size of the DSI > buffer. > > Could you please be more specific, which 'meaningless positive values' were > you receiving? > Sorry, I misread the code. I thought it returned negatives or the number of bytes copied. (This is from static analysis btw). Anyway, returning only negatives is a much better way. I will fix this patch and resend. regards, dan carpenter
