Re: [Freedreno] [PATCH v2 0/4] drm/msm: improve register snapshotting

2021-04-27 Thread abhinavk 

On 2021-04-26 17:18, Dmitry Baryshkov wrote:

Rework MSM coredump support: add DSI PHY registers, simplify
snapshotting code.

Changes since v1:
 - Readd mutex serializing register snapshot calls

 - Add DSI PHY register dumping support


Need to mention the dependency here , got missed from the prev patchset


Dmitry Baryshkov (4):
  drm/msm: pass dump state as a function argument
  drm/msm: make msm_disp_state transient data struct
  drm/msm: get rid of msm_iomap_size
  drm/msm/dsi: add DSI PHY registers to snapshot data

 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  5 +-
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c  | 90 
+++

 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h  | 21 +-
 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 22 ++
 drivers/gpu/drm/msm/dp/dp_display.c   |  4 +-
 drivers/gpu/drm/msm/dsi/dsi.c |  5 +-
 drivers/gpu/drm/msm/dsi/dsi.h |  5 +-
 drivers/gpu/drm/msm/dsi/dsi_host.c| 11 +--
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 31 +++-
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h |  4 +
 drivers/gpu/drm/msm/msm_drv.c | 27 +++
 drivers/gpu/drm/msm/msm_drv.h |  6 +-
 drivers/gpu/drm/msm/msm_kms.h |  8 +-
 13 files changed, 97 insertions(+), 142 deletions(-)


___
dri-devel mailing list
dri-de...@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v2 2/4] drm/msm: make msm_disp_state transient data struct

2021-04-27 Thread abhinavk 

Hi Dmitry

On 2021-04-26 17:18, Dmitry Baryshkov wrote:

Instead of allocating snapshotting structure at the driver probe time
and later handling concurrent access, actual state, etc, make
msm_disp_state transient struct. Allocate one when snapshotting happens
and free it after coredump data is read by userspace.


Can you please check my previous comment on coredump_pending?

https://lore.kernel.org/dri-devel/186825e2fb7bea8d45f33b5c1fa35...@codeaurora.org/T/#u

That helps to serialize read/write of coredump.

Rest of the changes on this one look fine to me.


Signed-off-by: Dmitry Baryshkov 
---
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.c  | 90 ++-
 drivers/gpu/drm/msm/disp/msm_disp_snapshot.h  | 13 +--
 .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |  5 +-
 drivers/gpu/drm/msm/msm_kms.h |  6 +-
 4 files changed, 37 insertions(+), 77 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
index 70fd5a1fe13e..a4a7cb06bc87 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
@@ -7,8 +7,7 @@

 #include "msm_disp_snapshot.h"

-#ifdef CONFIG_DEV_COREDUMP
-static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
+static ssize_t __maybe_unused disp_devcoredump_read(char *buffer,
loff_t offset,
size_t count, void *data, size_t datalen)
 {
struct drm_print_iterator iter;
@@ -29,52 +28,47 @@ static ssize_t disp_devcoredump_read(char *buffer,
loff_t offset,
return count - iter.remain;
 }

-static void disp_devcoredump_free(void *data)
+static void _msm_disp_snapshot_work(struct kthread_work *work)
 {
+   struct msm_kms *kms = container_of(work, struct msm_kms, dump_work);
+   struct drm_device *drm_dev = kms->dev;
struct msm_disp_state *disp_state;
+   struct drm_printer p;

-   disp_state = data;
-
-   msm_disp_state_free(disp_state);
+   disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
+   if (!disp_state)
+   return;

-   disp_state->coredump_pending = false;
-}
-#endif /* CONFIG_DEV_COREDUMP */
+   disp_state->dev = drm_dev->dev;
+   disp_state->drm_dev = drm_dev;

-static void _msm_disp_snapshot_work(struct kthread_work *work)
-{
-   struct msm_disp_state *disp_state = container_of(work, struct
msm_disp_state, dump_work);
-   struct drm_printer p;
+   INIT_LIST_HEAD(&disp_state->blocks);

-   mutex_lock(&disp_state->mutex);
+   /* Serialize dumping here */
+   mutex_lock(&kms->dump_mutex);

msm_disp_snapshot_capture_state(disp_state);

+   mutex_unlock(&kms->dump_mutex);
+
if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
p = drm_info_printer(disp_state->drm_dev->dev);
msm_disp_state_print(disp_state, &p);
}

/*
-* if devcoredump is not defined free the state immediately
-* otherwise it will be freed in the free handler.
+* If COREDUMP is disabled, the stub will call the free function.
+* If there is a codedump pending for the device, the dev_coredumpm()
+* will also free new coredump state.
 */
-#ifdef CONFIG_DEV_COREDUMP
 	dev_coredumpm(disp_state->dev, THIS_MODULE, disp_state, 0, 
GFP_KERNEL,

-   disp_devcoredump_read, disp_devcoredump_free);
-   disp_state->coredump_pending = true;
-#else
-   msm_disp_state_free(disp_state);
-#endif
-
-   mutex_unlock(&disp_state->mutex);
+   disp_devcoredump_read, msm_disp_state_free);
 }

 void msm_disp_snapshot_state(struct drm_device *drm_dev)
 {
struct msm_drm_private *priv;
struct msm_kms *kms;
-   struct msm_disp_state *disp_state;

if (!drm_dev) {
DRM_ERROR("invalid params\n");
@@ -83,30 +77,13 @@ void msm_disp_snapshot_state(struct drm_device 
*drm_dev)


priv = drm_dev->dev_private;
kms = priv->kms;
-   disp_state = kms->disp_state;
-
-   if (!disp_state) {
-   DRM_ERROR("invalid params\n");
-   return;
-   }

-   /*
-* if there is a coredump pending return immediately till dump
-* if read by userspace or timeout happens
-*/
-   if (disp_state->coredump_pending) {
-   DRM_DEBUG("coredump is pending read\n");
-   return;
-   }
-
-   kthread_queue_work(disp_state->dump_worker,
-   &disp_state->dump_work);
+   kthread_queue_work(kms->dump_worker, &kms->dump_work);
 }

 int msm_disp_snapshot_init(struct drm_device *drm_dev)
 {
struct msm_drm_private *priv;
-   struct msm_disp_state *disp_state;
struct msm_kms *kms;

if (!drm_dev) {
@@ -117,22 +94,13 @@ int msm_disp_snapshot_init(struct drm_device 
*drm_dev)

priv = drm_dev->dev_private;
kms = priv->kms;

-   disp_state = devm_kzalloc(drm_dev->

Re: [Freedreno] [PATCH v2 3/4] drm/msm: get rid of msm_iomap_size

2021-04-27 Thread abhinavk 

Hi Dmitry

On 2021-04-26 17:18, Dmitry Baryshkov wrote:

Instead of looping throught the resources each time to get the DSI CTRL
area size, get it at the ioremap time.

Signed-off-by: Dmitry Baryshkov 

We will have to call into the individual modules anyway everytime we
take a snapshot as only they have access to the required clocks and the 
base address.


So even though there is nothing wrong with this change, it still adds a 
size member

which can be avoided because we have to call into the module anyway.

Any strong preference to store the size as opposed to just getting it 
when we take

the snapshot?


---
 drivers/gpu/drm/msm/dsi/dsi_host.c |  5 +++--
 drivers/gpu/drm/msm/msm_drv.c  | 27 +--
 drivers/gpu/drm/msm/msm_drv.h  |  3 ++-
 3 files changed, 14 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 1a63368c3912..b3ee5c0bce12 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -102,6 +102,7 @@ struct msm_dsi_host {
int id;

void __iomem *ctrl_base;
+   phys_addr_t ctrl_size;
struct regulator_bulk_data supplies[DSI_DEV_REGULATOR_MAX];

struct clk *bus_clks[DSI_BUS_CLK_MAX];
@@ -1839,7 +1840,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
goto fail;
}

-   msm_host->ctrl_base = msm_ioremap(pdev, "dsi_ctrl", "DSI CTRL");
+   msm_host->ctrl_base = msm_ioremap_size(pdev, "dsi_ctrl", "DSI CTRL",
&msm_host->ctrl_size);
if (IS_ERR(msm_host->ctrl_base)) {
pr_err("%s: unable to map Dsi ctrl base\n", __func__);
ret = PTR_ERR(msm_host->ctrl_base);
@@ -2494,7 +2495,7 @@ void msm_dsi_host_snapshot(struct msm_disp_state
*disp_state, struct mipi_dsi_ho

pm_runtime_get_sync(&msm_host->pdev->dev);

-   msm_disp_snapshot_add_block(disp_state,
msm_iomap_size(msm_host->pdev, "dsi_ctrl"),
+   msm_disp_snapshot_add_block(disp_state, msm_host->ctrl_size,
msm_host->ctrl_base, "dsi%d_ctrl", msm_host->id);

pm_runtime_put_sync(&msm_host->pdev->dev);
diff --git a/drivers/gpu/drm/msm/msm_drv.c 
b/drivers/gpu/drm/msm/msm_drv.c

index 92fe844b517b..be578fc4e54f 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -124,7 +124,7 @@ struct clk *msm_clk_get(struct platform_device
*pdev, const char *name)
 }

 static void __iomem *_msm_ioremap(struct platform_device *pdev, const
char *name,
- const char *dbgname, bool quiet)
+ const char *dbgname, bool quiet, phys_addr_t 
*psize)
 {
struct resource *res;
unsigned long size;
@@ -153,37 +153,28 @@ static void __iomem *_msm_ioremap(struct
platform_device *pdev, const char *name
if (reglog)
printk(KERN_DEBUG "IO:region %s %p %08lx\n", dbgname, ptr, 
size);

+   if (psize)
+   *psize = size;
+
return ptr;
 }

 void __iomem *msm_ioremap(struct platform_device *pdev, const char 
*name,

  const char *dbgname)
 {
-   return _msm_ioremap(pdev, name, dbgname, false);
+   return _msm_ioremap(pdev, name, dbgname, false, NULL);
 }

 void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const 
char *name,

const char *dbgname)
 {
-   return _msm_ioremap(pdev, name, dbgname, true);
+   return _msm_ioremap(pdev, name, dbgname, true, NULL);
 }

-unsigned long msm_iomap_size(struct platform_device *pdev, const char 
*name)
+void __iomem *msm_ioremap_size(struct platform_device *pdev, const 
char *name,

+ const char *dbgname, phys_addr_t *psize)
 {
-   struct resource *res;
-
-   if (name)
-   res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
-   else
-   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-
-   if (!res) {
-   dev_dbg(&pdev->dev, "failed to get memory resource: %s\n",
-   name);
-   return 0;
-   }
-
-   return resource_size(res);
+   return _msm_ioremap(pdev, name, dbgname, false, psize);
 }

 void msm_writel(u32 data, void __iomem *addr)
diff --git a/drivers/gpu/drm/msm/msm_drv.h 
b/drivers/gpu/drm/msm/msm_drv.h

index 15cb34451ded..c33fc1293789 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -450,9 +450,10 @@ struct clk *msm_clk_bulk_get_clock(struct
clk_bulk_data *bulk, int count,
const char *name);
 void __iomem *msm_ioremap(struct platform_device *pdev, const char 
*name,

const char *dbgname);
+void __iomem *msm_ioremap_size(struct platform_device *pdev, const 
char *name,

+   const char *dbgname, phys_addr_t *size);
 void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const 
char *name,

const char *dbgn

Re: [Freedreno] [PATCH v2 2/4] drm/msm: make msm_disp_state transient data struct

2021-04-27 Thread Dmitry Baryshkov 
On Tue, 27 Apr 2021 at 22:19,  wrote:
>
> Hi Dmitry
>
> On 2021-04-26 17:18, Dmitry Baryshkov wrote:
> > Instead of allocating snapshotting structure at the driver probe time
> > and later handling concurrent access, actual state, etc, make
> > msm_disp_state transient struct. Allocate one when snapshotting happens
> > and free it after coredump data is read by userspace.
> >
> Can you please check my previous comment on coredump_pending?
>
> https://lore.kernel.org/dri-devel/186825e2fb7bea8d45f33b5c1fa35...@codeaurora.org/T/#u
>
> That helps to serialize read/write of coredump.

Regarding the serialization of read/write. As the disp_state becomes
the transient object, the need for such serialization vanishes:
 - Before the snapshot is complete, the object is not linked outside
of msm_disp_snapshot functions.
 - When the snapshot is complete, it becomes linked from the codedump
subsystem. After this it is only read by the coredump, nobody will
write to it.
 - Next snapshot will allocate a new disp_state structure.
 - If dev_coredumpm is called when the previous snapshot is still
referenced (the device exists), the new snapshot is silently freed.

Thus there is no need to sync the read/write operations. They are now
naturally serialized.

>
> Rest of the changes on this one look fine to me.
>
> > Signed-off-by: Dmitry Baryshkov 
> > ---
> >  drivers/gpu/drm/msm/disp/msm_disp_snapshot.c  | 90 ++-
> >  drivers/gpu/drm/msm/disp/msm_disp_snapshot.h  | 13 +--
> >  .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |  5 +-
> >  drivers/gpu/drm/msm/msm_kms.h |  6 +-
> >  4 files changed, 37 insertions(+), 77 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> > b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> > index 70fd5a1fe13e..a4a7cb06bc87 100644
> > --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> > +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> > @@ -7,8 +7,7 @@
> >
> >  #include "msm_disp_snapshot.h"
> >
> > -#ifdef CONFIG_DEV_COREDUMP
> > -static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
> > +static ssize_t __maybe_unused disp_devcoredump_read(char *buffer,
> > loff_t offset,
> >   size_t count, void *data, size_t datalen)
> >  {
> >   struct drm_print_iterator iter;
> > @@ -29,52 +28,47 @@ static ssize_t disp_devcoredump_read(char *buffer,
> > loff_t offset,
> >   return count - iter.remain;
> >  }
> >
> > -static void disp_devcoredump_free(void *data)
> > +static void _msm_disp_snapshot_work(struct kthread_work *work)
> >  {
> > + struct msm_kms *kms = container_of(work, struct msm_kms, dump_work);
> > + struct drm_device *drm_dev = kms->dev;
> >   struct msm_disp_state *disp_state;
> > + struct drm_printer p;
> >
> > - disp_state = data;
> > -
> > - msm_disp_state_free(disp_state);
> > + disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
> > + if (!disp_state)
> > + return;
> >
> > - disp_state->coredump_pending = false;
> > -}
> > -#endif /* CONFIG_DEV_COREDUMP */
> > + disp_state->dev = drm_dev->dev;
> > + disp_state->drm_dev = drm_dev;
> >
> > -static void _msm_disp_snapshot_work(struct kthread_work *work)
> > -{
> > - struct msm_disp_state *disp_state = container_of(work, struct
> > msm_disp_state, dump_work);
> > - struct drm_printer p;
> > + INIT_LIST_HEAD(&disp_state->blocks);
> >
> > - mutex_lock(&disp_state->mutex);
> > + /* Serialize dumping here */
> > + mutex_lock(&kms->dump_mutex);
> >
> >   msm_disp_snapshot_capture_state(disp_state);
> >
> > + mutex_unlock(&kms->dump_mutex);
> > +
> >   if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
> >   p = drm_info_printer(disp_state->drm_dev->dev);
> >   msm_disp_state_print(disp_state, &p);
> >   }
> >
> >   /*
> > -  * if devcoredump is not defined free the state immediately
> > -  * otherwise it will be freed in the free handler.
> > +  * If COREDUMP is disabled, the stub will call the free function.
> > +  * If there is a codedump pending for the device, the dev_coredumpm()
> > +  * will also free new coredump state.
> >*/
> > -#ifdef CONFIG_DEV_COREDUMP
> >   dev_coredumpm(disp_state->dev, THIS_MODULE, disp_state, 0,
> > GFP_KERNEL,
> > - disp_devcoredump_read, disp_devcoredump_free);
> > - disp_state->coredump_pending = true;
> > -#else
> > - msm_disp_state_free(disp_state);
> > -#endif
> > -
> > - mutex_unlock(&disp_state->mutex);
> > + disp_devcoredump_read, msm_disp_state_free);
> >  }
> >
> >  void msm_disp_snapshot_state(struct drm_device *drm_dev)
> >  {
> >   struct msm_drm_private *priv;
> >   struct msm_kms *kms;
> > - struct msm_disp_state *disp_state;
> >
> >   if (!drm_dev) {
> >   DRM_ERROR("invalid params\n");
> > @@ -83,30 +77,13 @@ void msm_disp_snapshot_state(struct drm_device
> > *drm_de

Re: [Freedreno] [PATCH v2 3/4] drm/msm: get rid of msm_iomap_size

2021-04-27 Thread Dmitry Baryshkov 
On Tue, 27 Apr 2021 at 22:30,  wrote:
>
> Hi Dmitry
>
> On 2021-04-26 17:18, Dmitry Baryshkov wrote:
> > Instead of looping throught the resources each time to get the DSI CTRL
> > area size, get it at the ioremap time.
> >
> > Signed-off-by: Dmitry Baryshkov 
> We will have to call into the individual modules anyway everytime we
> take a snapshot as only they have access to the required clocks and the
> base address.
>
> So even though there is nothing wrong with this change, it still adds a
> size member
> which can be avoided because we have to call into the module anyway.
>
> Any strong preference to store the size as opposed to just getting it
> when we take
> the snapshot?

Locality. We ioremap the resource from one piece of code and then we
get it's length from a completely different piece of code. If we ever
change e.g. the ioremap'ed resource name, we'd have to also update
snapshot users.
With this patch these changes are done in a transparent way. Whichever
we do with the regions in future, there is always a valid base + size
combo.

>
> > ---
> >  drivers/gpu/drm/msm/dsi/dsi_host.c |  5 +++--
> >  drivers/gpu/drm/msm/msm_drv.c  | 27 +--
> >  drivers/gpu/drm/msm/msm_drv.h  |  3 ++-
> >  3 files changed, 14 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index 1a63368c3912..b3ee5c0bce12 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -102,6 +102,7 @@ struct msm_dsi_host {
> >   int id;
> >
> >   void __iomem *ctrl_base;
> > + phys_addr_t ctrl_size;
> >   struct regulator_bulk_data supplies[DSI_DEV_REGULATOR_MAX];
> >
> >   struct clk *bus_clks[DSI_BUS_CLK_MAX];
> > @@ -1839,7 +1840,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
> >   goto fail;
> >   }
> >
> > - msm_host->ctrl_base = msm_ioremap(pdev, "dsi_ctrl", "DSI CTRL");
> > + msm_host->ctrl_base = msm_ioremap_size(pdev, "dsi_ctrl", "DSI CTRL",
> > &msm_host->ctrl_size);
> >   if (IS_ERR(msm_host->ctrl_base)) {
> >   pr_err("%s: unable to map Dsi ctrl base\n", __func__);
> >   ret = PTR_ERR(msm_host->ctrl_base);
> > @@ -2494,7 +2495,7 @@ void msm_dsi_host_snapshot(struct msm_disp_state
> > *disp_state, struct mipi_dsi_ho
> >
> >   pm_runtime_get_sync(&msm_host->pdev->dev);
> >
> > - msm_disp_snapshot_add_block(disp_state,
> > msm_iomap_size(msm_host->pdev, "dsi_ctrl"),
> > + msm_disp_snapshot_add_block(disp_state, msm_host->ctrl_size,
> >   msm_host->ctrl_base, "dsi%d_ctrl", msm_host->id);
> >
> >   pm_runtime_put_sync(&msm_host->pdev->dev);
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c
> > b/drivers/gpu/drm/msm/msm_drv.c
> > index 92fe844b517b..be578fc4e54f 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -124,7 +124,7 @@ struct clk *msm_clk_get(struct platform_device
> > *pdev, const char *name)
> >  }
> >
> >  static void __iomem *_msm_ioremap(struct platform_device *pdev, const
> > char *name,
> > -   const char *dbgname, bool quiet)
> > +   const char *dbgname, bool quiet, 
> > phys_addr_t *psize)
> >  {
> >   struct resource *res;
> >   unsigned long size;
> > @@ -153,37 +153,28 @@ static void __iomem *_msm_ioremap(struct
> > platform_device *pdev, const char *name
> >   if (reglog)
> >   printk(KERN_DEBUG "IO:region %s %p %08lx\n", dbgname, ptr, 
> > size);
> >
> > + if (psize)
> > + *psize = size;
> > +
> >   return ptr;
> >  }
> >
> >  void __iomem *msm_ioremap(struct platform_device *pdev, const char
> > *name,
> > const char *dbgname)
> >  {
> > - return _msm_ioremap(pdev, name, dbgname, false);
> > + return _msm_ioremap(pdev, name, dbgname, false, NULL);
> >  }
> >
> >  void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const
> > char *name,
> >   const char *dbgname)
> >  {
> > - return _msm_ioremap(pdev, name, dbgname, true);
> > + return _msm_ioremap(pdev, name, dbgname, true, NULL);
> >  }
> >
> > -unsigned long msm_iomap_size(struct platform_device *pdev, const char
> > *name)
> > +void __iomem *msm_ioremap_size(struct platform_device *pdev, const
> > char *name,
> > +   const char *dbgname, phys_addr_t *psize)
> >  {
> > - struct resource *res;
> > -
> > - if (name)
> > - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, 
> > name);
> > - else
> > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -
> > - if (!res) {
> > - dev_dbg(&pdev->dev, "failed to get memory resource: %s\n",
> > - name);
> > - return 0;
> > - }
> > -
> > - return resource_size(res);
> > + return _msm_ioremap(pdev,

Re: [Freedreno] [PATCH v2 2/4] drm/msm: make msm_disp_state transient data struct

2021-04-27 Thread abhinavk 

On 2021-04-27 13:29, Dmitry Baryshkov wrote:

On Tue, 27 Apr 2021 at 22:19,  wrote:


Hi Dmitry

On 2021-04-26 17:18, Dmitry Baryshkov wrote:
> Instead of allocating snapshotting structure at the driver probe time
> and later handling concurrent access, actual state, etc, make
> msm_disp_state transient struct. Allocate one when snapshotting happens
> and free it after coredump data is read by userspace.
>
Can you please check my previous comment on coredump_pending?

https://lore.kernel.org/dri-devel/186825e2fb7bea8d45f33b5c1fa35...@codeaurora.org/T/#u

That helps to serialize read/write of coredump.


Regarding the serialization of read/write. As the disp_state becomes
the transient object, the need for such serialization vanishes:
 - Before the snapshot is complete, the object is not linked outside
of msm_disp_snapshot functions.
 - When the snapshot is complete, it becomes linked from the codedump
subsystem. After this it is only read by the coredump, nobody will
write to it.
 - Next snapshot will allocate a new disp_state structure.
 - If dev_coredumpm is called when the previous snapshot is still
referenced (the device exists), the new snapshot is silently freed.

Thus there is no need to sync the read/write operations. They are now
naturally serialized.
Alright, just make sure to validate the robustness of this using the 
method mentioned earlier.

Apart from that,

Reviewed-by: Abhinav Kumar 




Rest of the changes on this one look fine to me.

> Signed-off-by: Dmitry Baryshkov 
> ---
>  drivers/gpu/drm/msm/disp/msm_disp_snapshot.c  | 90 ++-
>  drivers/gpu/drm/msm/disp/msm_disp_snapshot.h  | 13 +--
>  .../gpu/drm/msm/disp/msm_disp_snapshot_util.c |  5 +-
>  drivers/gpu/drm/msm/msm_kms.h |  6 +-
>  4 files changed, 37 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> index 70fd5a1fe13e..a4a7cb06bc87 100644
> --- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> +++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot.c
> @@ -7,8 +7,7 @@
>
>  #include "msm_disp_snapshot.h"
>
> -#ifdef CONFIG_DEV_COREDUMP
> -static ssize_t disp_devcoredump_read(char *buffer, loff_t offset,
> +static ssize_t __maybe_unused disp_devcoredump_read(char *buffer,
> loff_t offset,
>   size_t count, void *data, size_t datalen)
>  {
>   struct drm_print_iterator iter;
> @@ -29,52 +28,47 @@ static ssize_t disp_devcoredump_read(char *buffer,
> loff_t offset,
>   return count - iter.remain;
>  }
>
> -static void disp_devcoredump_free(void *data)
> +static void _msm_disp_snapshot_work(struct kthread_work *work)
>  {
> + struct msm_kms *kms = container_of(work, struct msm_kms, dump_work);
> + struct drm_device *drm_dev = kms->dev;
>   struct msm_disp_state *disp_state;
> + struct drm_printer p;
>
> - disp_state = data;
> -
> - msm_disp_state_free(disp_state);
> + disp_state = kzalloc(sizeof(struct msm_disp_state), GFP_KERNEL);
> + if (!disp_state)
> + return;
>
> - disp_state->coredump_pending = false;
> -}
> -#endif /* CONFIG_DEV_COREDUMP */
> + disp_state->dev = drm_dev->dev;
> + disp_state->drm_dev = drm_dev;
>
> -static void _msm_disp_snapshot_work(struct kthread_work *work)
> -{
> - struct msm_disp_state *disp_state = container_of(work, struct
> msm_disp_state, dump_work);
> - struct drm_printer p;
> + INIT_LIST_HEAD(&disp_state->blocks);
>
> - mutex_lock(&disp_state->mutex);
> + /* Serialize dumping here */
> + mutex_lock(&kms->dump_mutex);
>
>   msm_disp_snapshot_capture_state(disp_state);
>
> + mutex_unlock(&kms->dump_mutex);
> +
>   if (MSM_DISP_SNAPSHOT_DUMP_IN_CONSOLE) {
>   p = drm_info_printer(disp_state->drm_dev->dev);
>   msm_disp_state_print(disp_state, &p);
>   }
>
>   /*
> -  * if devcoredump is not defined free the state immediately
> -  * otherwise it will be freed in the free handler.
> +  * If COREDUMP is disabled, the stub will call the free function.
> +  * If there is a codedump pending for the device, the dev_coredumpm()
> +  * will also free new coredump state.
>*/
> -#ifdef CONFIG_DEV_COREDUMP
>   dev_coredumpm(disp_state->dev, THIS_MODULE, disp_state, 0,
> GFP_KERNEL,
> - disp_devcoredump_read, disp_devcoredump_free);
> - disp_state->coredump_pending = true;
> -#else
> - msm_disp_state_free(disp_state);
> -#endif
> -
> - mutex_unlock(&disp_state->mutex);
> + disp_devcoredump_read, msm_disp_state_free);
>  }
>
>  void msm_disp_snapshot_state(struct drm_device *drm_dev)
>  {
>   struct msm_drm_private *priv;
>   struct msm_kms *kms;
> - struct msm_disp_state *disp_state;
>
>   if (!drm_dev) {
>   DRM_ERROR("invalid params\n");
> @@ -83,30 +77,13 @@ void msm_disp_snapshot_state(struct drm_device
> *drm_dev)
>
>   priv = drm_dev->dev_p

Re: [Freedreno] [PATCH v2 3/4] drm/msm: get rid of msm_iomap_size

2021-04-27 Thread abhinavk 

On 2021-04-27 13:32, Dmitry Baryshkov wrote:

On Tue, 27 Apr 2021 at 22:30,  wrote:


Hi Dmitry

On 2021-04-26 17:18, Dmitry Baryshkov wrote:
> Instead of looping throught the resources each time to get the DSI CTRL
> area size, get it at the ioremap time.
>
> Signed-off-by: Dmitry Baryshkov 
We will have to call into the individual modules anyway everytime we
take a snapshot as only they have access to the required clocks and 
the

base address.

So even though there is nothing wrong with this change, it still adds 
a

size member
which can be avoided because we have to call into the module anyway.

Any strong preference to store the size as opposed to just getting it
when we take
the snapshot?


Locality. We ioremap the resource from one piece of code and then we
get it's length from a completely different piece of code. If we ever
change e.g. the ioremap'ed resource name, we'd have to also update
snapshot users.
With this patch these changes are done in a transparent way. Whichever
we do with the regions in future, there is always a valid base + size
combo.


Alright, no further concerns from my side on this:

Reviewed-by: Abhinav Kumar 



> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c |  5 +++--
>  drivers/gpu/drm/msm/msm_drv.c  | 27 +--
>  drivers/gpu/drm/msm/msm_drv.h  |  3 ++-
>  3 files changed, 14 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 1a63368c3912..b3ee5c0bce12 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -102,6 +102,7 @@ struct msm_dsi_host {
>   int id;
>
>   void __iomem *ctrl_base;
> + phys_addr_t ctrl_size;
>   struct regulator_bulk_data supplies[DSI_DEV_REGULATOR_MAX];
>
>   struct clk *bus_clks[DSI_BUS_CLK_MAX];
> @@ -1839,7 +1840,7 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
>   goto fail;
>   }
>
> - msm_host->ctrl_base = msm_ioremap(pdev, "dsi_ctrl", "DSI CTRL");
> + msm_host->ctrl_base = msm_ioremap_size(pdev, "dsi_ctrl", "DSI CTRL",
> &msm_host->ctrl_size);
>   if (IS_ERR(msm_host->ctrl_base)) {
>   pr_err("%s: unable to map Dsi ctrl base\n", __func__);
>   ret = PTR_ERR(msm_host->ctrl_base);
> @@ -2494,7 +2495,7 @@ void msm_dsi_host_snapshot(struct msm_disp_state
> *disp_state, struct mipi_dsi_ho
>
>   pm_runtime_get_sync(&msm_host->pdev->dev);
>
> - msm_disp_snapshot_add_block(disp_state,
> msm_iomap_size(msm_host->pdev, "dsi_ctrl"),
> + msm_disp_snapshot_add_block(disp_state, msm_host->ctrl_size,
>   msm_host->ctrl_base, "dsi%d_ctrl", msm_host->id);
>
>   pm_runtime_put_sync(&msm_host->pdev->dev);
> diff --git a/drivers/gpu/drm/msm/msm_drv.c
> b/drivers/gpu/drm/msm/msm_drv.c
> index 92fe844b517b..be578fc4e54f 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -124,7 +124,7 @@ struct clk *msm_clk_get(struct platform_device
> *pdev, const char *name)
>  }
>
>  static void __iomem *_msm_ioremap(struct platform_device *pdev, const
> char *name,
> -   const char *dbgname, bool quiet)
> +   const char *dbgname, bool quiet, phys_addr_t 
*psize)
>  {
>   struct resource *res;
>   unsigned long size;
> @@ -153,37 +153,28 @@ static void __iomem *_msm_ioremap(struct
> platform_device *pdev, const char *name
>   if (reglog)
>   printk(KERN_DEBUG "IO:region %s %p %08lx\n", dbgname, ptr, 
size);
>
> + if (psize)
> + *psize = size;
> +
>   return ptr;
>  }
>
>  void __iomem *msm_ioremap(struct platform_device *pdev, const char
> *name,
> const char *dbgname)
>  {
> - return _msm_ioremap(pdev, name, dbgname, false);
> + return _msm_ioremap(pdev, name, dbgname, false, NULL);
>  }
>
>  void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const
> char *name,
>   const char *dbgname)
>  {
> - return _msm_ioremap(pdev, name, dbgname, true);
> + return _msm_ioremap(pdev, name, dbgname, true, NULL);
>  }
>
> -unsigned long msm_iomap_size(struct platform_device *pdev, const char
> *name)
> +void __iomem *msm_ioremap_size(struct platform_device *pdev, const
> char *name,
> +   const char *dbgname, phys_addr_t *psize)
>  {
> - struct resource *res;
> -
> - if (name)
> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> - else
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -
> - if (!res) {
> - dev_dbg(&pdev->dev, "failed to get memory resource: %s\n",
> - name);
> - return 0;
> - }
> -
> - return resource_size(res);
> + return _msm_ioremap(pdev, name, dbgname, false, psize);
>  }
>
>  void msm_writel(u32 data, void __iomem *addr)
> diff --git a/drivers/gpu/drm/msm

Re: [Freedreno] [PATCH v2 4/4] drm/msm/dsi: add DSI PHY registers to snapshot data

2021-04-27 Thread abhinavk 

On 2021-04-26 17:18, Dmitry Baryshkov wrote:

Add DSI PHY registers to the msm state snapshots to be able to check
their contents.

Signed-off-by: Dmitry Baryshkov 


Reviewed-by: Abhinav Kumar 

---
 drivers/gpu/drm/msm/dsi/dsi.c |  1 +
 drivers/gpu/drm/msm/dsi/dsi.h |  1 +
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 31 +++
 drivers/gpu/drm/msm/dsi/phy/dsi_phy.h |  4 
 4 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.c 
b/drivers/gpu/drm/msm/dsi/dsi.c

index 322d2e535df0..75afc12a7b25 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -269,5 +269,6 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi,
struct drm_device *dev,
 void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct
msm_dsi *msm_dsi)
 {
msm_dsi_host_snapshot(disp_state, msm_dsi->host);
+   msm_dsi_phy_snapshot(disp_state, msm_dsi->phy);
 }

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
b/drivers/gpu/drm/msm/dsi/dsi.h

index b5679cf89413..cea73f9c4be9 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -176,6 +176,7 @@ int msm_dsi_phy_get_clk_provider(struct msm_dsi_phy 
*phy,

struct clk **byte_clk_provider, struct clk **pixel_clk_provider);
 void msm_dsi_phy_pll_save_state(struct msm_dsi_phy *phy);
 int msm_dsi_phy_pll_restore_state(struct msm_dsi_phy *phy);
+void msm_dsi_phy_snapshot(struct msm_disp_state *disp_state, struct
msm_dsi_phy *phy);

 #endif /* __DSI_CONNECTOR_H__ */

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
index f0a2ddf96a4b..bf7a4c20c13c 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c
@@ -658,14 +658,14 @@ static int dsi_phy_driver_probe(struct
platform_device *pdev)
phy->regulator_ldo_mode = of_property_read_bool(dev->of_node,
"qcom,dsi-phy-regulator-ldo-mode");

-   phy->base = msm_ioremap(pdev, "dsi_phy", "DSI_PHY");
+	phy->base = msm_ioremap_size(pdev, "dsi_phy", "DSI_PHY", 
&phy->base_size);

if (IS_ERR(phy->base)) {
DRM_DEV_ERROR(dev, "%s: failed to map phy base\n", __func__);
ret = -ENOMEM;
goto fail;
}

-   phy->pll_base = msm_ioremap(pdev, "dsi_pll", "DSI_PLL");
+	phy->pll_base = msm_ioremap_size(pdev, "dsi_pll", "DSI_PLL", 
&phy->pll_size);

if (IS_ERR(phy->pll_base)) {
DRM_DEV_ERROR(&pdev->dev, "%s: failed to map pll base\n", 
__func__);
ret = -ENOMEM;
@@ -673,7 +673,7 @@ static int dsi_phy_driver_probe(struct
platform_device *pdev)
}

if (phy->cfg->has_phy_lane) {
-   phy->lane_base = msm_ioremap(pdev, "dsi_phy_lane", 
"DSI_PHY_LANE");
+   phy->lane_base = msm_ioremap_size(pdev, "dsi_phy_lane",
"DSI_PHY_LANE", &phy->lane_size);
if (IS_ERR(phy->lane_base)) {
 			DRM_DEV_ERROR(&pdev->dev, "%s: failed to map phy lane base\n", 
__func__);

ret = -ENOMEM;
@@ -682,7 +682,7 @@ static int dsi_phy_driver_probe(struct
platform_device *pdev)
}

if (phy->cfg->has_phy_regulator) {
-		phy->reg_base = msm_ioremap(pdev, "dsi_phy_regulator", 
"DSI_PHY_REG");

+   phy->reg_base = msm_ioremap_size(pdev, "dsi_phy_regulator",
"DSI_PHY_REG", &phy->reg_size);
if (IS_ERR(phy->reg_base)) {
DRM_DEV_ERROR(&pdev->dev, "%s: failed to map phy 
regulator
base\n", __func__);
ret = -ENOMEM;
@@ -868,3 +868,26 @@ int msm_dsi_phy_pll_restore_state(struct 
msm_dsi_phy *phy)


return 0;
 }
+
+void msm_dsi_phy_snapshot(struct msm_disp_state *disp_state, struct
msm_dsi_phy *phy)
+{
+   msm_disp_snapshot_add_block(disp_state,
+   phy->base_size, phy->base,
+   "dsi%d_phy", phy->id);
+
+   /* Do not try accessing PLL registers if it is switched off */
+   if (phy->pll_on)
+   msm_disp_snapshot_add_block(disp_state,
+   phy->pll_size, phy->pll_base,
+   "dsi%d_pll", phy->id);
+
+   if (phy->lane_base)
+   msm_disp_snapshot_add_block(disp_state,
+   phy->lane_size, phy->lane_base,
+   "dsi%d_lane", phy->id);
+
+   if (phy->reg_base)
+   msm_disp_snapshot_add_block(disp_state,
+   phy->reg_size, phy->reg_base,
+   "dsi%d_reg", phy->id);
+}
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
index 94a77ac364d3..5b0feef87127 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h
@@ -85,6 +85,10 @@ struct msm_dsi_phy {
void __iomem *pll_base;
void __iomem *reg_base;
void __iomem *lane_base;
+   phys_addr_t base_size;
+   phys_addr_t pll_size;
+

Re: [Freedreno] [PATCH 1/2] drm/msm/dp: service only one irq_hpd if there are multiple irq_hpd pending

2021-04-27 Thread Stephen Boyd 
Quoting aravi...@codeaurora.org (2021-04-21 11:55:21)
> On 2021-04-21 10:26, khs...@codeaurora.org wrote:
> >>
> >>> +
> >>> mutex_unlock(&dp->event_mutex);
> >>>
> >>> return 0;
> >>> @@ -1496,6 +1502,9 @@ int msm_dp_display_disable(struct msm_dp *dp,
> >>> struct drm_encoder *encoder)
> >>> /* stop sentinel checking */
> >>> dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
> >>>
> >>> +   /* link is down, delete pending irq_hdps */
> >>> +   dp_del_event(dp_display, EV_IRQ_HPD_INT);
> >>> +
> >>
> >> I'm becoming convinced that the whole kthread design and event queue
> >> is
> >> broken. These sorts of patches are working around the larger problem
> >> that the kthread is running independently of the driver and irqs can
> >> come in at any time but the event queue is not checked from the irq
> >> handler to debounce the irq event. Is the event queue necessary at
> >> all?
> >> I wonder if it would be simpler to just use an irq thread and process
> >> the hpd signal from there. Then we're guaranteed to not get an irq
> >> again
> >> until the irq thread is done processing the event. This would
> >> naturally
> >> debounce the irq hpd event that way.
> > event q just like bottom half of irq handler. it turns irq into event
> > and handle them sequentially.
> > irq_hpd is asynchronous event from panel to bring up attention of hsot
> > during run time of operation.
> > Here, the dongle is unplugged and main link had teared down so that no
> > need to service pending irq_hpd if any.
> >
>
> As Kuogee mentioned, IRQ_HPD is a message received from the panel and is
> not like your typical HW generated IRQ. There is no guarantee that we
> will not receive an IRQ_HPD until we are finished with processing of an
> earlier HPD message or an IRQ_HPD message. For example - when you run
> the protocol compliance, when we get a HPD from the sink, we are
> expected to start reading DPCD, EDID and proceed with link training. As
> soon as link training is finished (which is marked by a specific DPCD
> register write), the sink is going to issue an IRQ_HPD. At this point,
> we may not done with processing the HPD high as after link training we
> would typically notify the user mode of the newly connected display,
> etc.

Given that the irq comes in and is then forked off to processing at a
later time implies that IRQ_HPD can come in at practically anytime. Case
in point, this patch, which is trying to selectively search through the
"event queue" and then remove the event that is no longer relevant
because the display is being turned off either by userspace or because
HPD has gone away. If we got rid of the queue and kthread and processed
irqs in a threaded irq handler I suspect the code would be simpler and
not have to search through an event queue when we disable the display.
Instead while disabling the display we would make sure that the irq
thread isn't running anymore with synchronize_irq() or even disable the
irq entirely, but really it would be better to just disable the irq in
the hardware with a register write to some irq mask register.

This pushes more of the logic for HPD and connect/disconnect into the
hardware and avoids reimplementing that in software: searching through
the queue, checking for duplicate events, etc.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v2 3/4] drm/msm: get rid of msm_iomap_size

2021-04-27 Thread Bjorn Andersson 
On Mon 26 Apr 19:18 CDT 2021, Dmitry Baryshkov wrote:
[..]
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 92fe844b517b..be578fc4e54f 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -124,7 +124,7 @@ struct clk *msm_clk_get(struct platform_device *pdev, 
> const char *name)
>  }
>  
>  static void __iomem *_msm_ioremap(struct platform_device *pdev, const char 
> *name,
> -   const char *dbgname, bool quiet)
> +   const char *dbgname, bool quiet, phys_addr_t 
> *psize)

size_t sounds like a better fit for psize...

Regards,
Bjorn
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno