Re: [Freedreno] [PATCH v4 5/6] drm: Add fdinfo memory stats

[Emil Velikov]

On Fri, 21 Apr 2023 at 12:23, Tvrtko Ursulin
 wrote:

> On 21/04/2023 11:26, Emil Velikov wrote:
> > On Wed, 12 Apr 2023 at 23:43, Rob Clark  wrote:
> >
> >> +/**
> >> + * enum drm_gem_object_status - bitmask of object state for fdinfo 
> >> reporting
> >> + * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not 
> >> unpinned)
> >> + * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
> >> + *
> >> + * Bitmask of status used for fdinfo memory stats, see 
> >> _gem_object_funcs.status
> >> + * and drm_show_fdinfo().  Note that an object can 
> >> DRM_GEM_OBJECT_PURGEABLE if
> >> + * it still active or not resident, in which case drm_show_fdinfo() will 
> >> not
> >
> > nit: s/can/can be/;s/if it still/if it is still/
> >
> >> + * account for it as purgeable.  So drivers do not need to check if the 
> >> buffer
> >> + * is idle and resident to return this bit.  (Ie. userspace can mark a 
> >> buffer
> >> + * as purgeable even while it is still busy on the GPU.. it does not 
> >> _actually_
> >> + * become puregeable until it becomes idle.  The status gem object func 
> >> does
> >
> > nit: s/puregeable/purgeable/
> >
> >
> > I think we want a similar note in the drm-usage-stats.rst file.
> >
> > With the above the whole series is:
> > Reviewed-by: Emil Velikov 
>
> Have you maybe noticed my slightly alternative proposal? (*) I am not a
> fan of putting this flavour of accounting into the core with no way to
> opt out. I think it leaves no option but to add more keys in the future
> for any driver which will not be happy with the core accounting.
>
> *) https://patchwork.freedesktop.org/series/116581/
>

Indeed I saw it. Not a fan of it, I'm afraid.

> > Fwiw: Keeping the i915 patch as part of this series would be great.
> > Sure i915_drm_client->id becomes dead code, but it's a piece one can
> > live with for a release or two. Then again it's not my call to make.
>
> Rob can take the i915 patch from my RFC too.
>

Indeed.

-Emil

Re: [Freedreno] [PATCH v4 5/6] drm: Add fdinfo memory stats

[Emil Velikov]

On Wed, 12 Apr 2023 at 23:43, Rob Clark  wrote:

> +/**
> + * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
> + * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
> + * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
> + *
> + * Bitmask of status used for fdinfo memory stats, see 
> _gem_object_funcs.status
> + * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE 
> if
> + * it still active or not resident, in which case drm_show_fdinfo() will not

nit: s/can/can be/;s/if it still/if it is still/

> + * account for it as purgeable.  So drivers do not need to check if the 
> buffer
> + * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
> + * as purgeable even while it is still busy on the GPU.. it does not 
> _actually_
> + * become puregeable until it becomes idle.  The status gem object func does

nit: s/puregeable/purgeable/


I think we want a similar note in the drm-usage-stats.rst file.

With the above the whole series is:
Reviewed-by: Emil Velikov 

Fwiw: Keeping the i915 patch as part of this series would be great.
Sure i915_drm_client->id becomes dead code, but it's a piece one can
live with for a release or two. Then again it's not my call to make.

HTH
Emil

Re: [Freedreno] [RFC 2/3] drm/msm: Rework get_comm_cmdline() helper

[Emil Velikov]

Greeting all,

Sorry for the delay - Easter Holidays, food coma and all that :-)

On Tue, 18 Apr 2023 at 15:31, Rob Clark  wrote:
>
> On Tue, Apr 18, 2023 at 1:34 AM Daniel Vetter  wrote:
> >
> > On Tue, Apr 18, 2023 at 09:27:49AM +0100, Tvrtko Ursulin wrote:
> > >
> > > On 17/04/2023 21:12, Rob Clark wrote:
> > > > From: Rob Clark 
> > > >
> > > > Make it work in terms of ctx so that it can be re-used for fdinfo.
> > > >
> > > > Signed-off-by: Rob Clark 
> > > > ---
> > > >   drivers/gpu/drm/msm/adreno/adreno_gpu.c |  4 ++--
> > > >   drivers/gpu/drm/msm/msm_drv.c   |  2 ++
> > > >   drivers/gpu/drm/msm/msm_gpu.c   | 13 ++---
> > > >   drivers/gpu/drm/msm/msm_gpu.h   | 12 ++--
> > > >   drivers/gpu/drm/msm/msm_submitqueue.c   |  1 +
> > > >   5 files changed, 21 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c 
> > > > b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > index bb38e728864d..43c4e1fea83f 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > > > @@ -412,7 +412,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct 
> > > > msm_file_private *ctx,
> > > > /* Ensure string is null terminated: */
> > > > str[len] = '\0';
> > > > -   mutex_lock(>lock);
> > > > +   mutex_lock(>lock);
> > > > if (param == MSM_PARAM_COMM) {
> > > > paramp = >comm;
> > > > @@ -423,7 +423,7 @@ int adreno_set_param(struct msm_gpu *gpu, struct 
> > > > msm_file_private *ctx,
> > > > kfree(*paramp);
> > > > *paramp = str;
> > > > -   mutex_unlock(>lock);
> > > > +   mutex_unlock(>lock);
> > > > return 0;
> > > > }
> > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c 
> > > > b/drivers/gpu/drm/msm/msm_drv.c
> > > > index 3d73b98d6a9c..ca0e89e46e13 100644
> > > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > > @@ -581,6 +581,8 @@ static int context_init(struct drm_device *dev, 
> > > > struct drm_file *file)
> > > > rwlock_init(>queuelock);
> > > > kref_init(>ref);
> > > > +   ctx->pid = get_pid(task_pid(current));
> > >
> > > Would it simplify things for msm if DRM core had an up to date file->pid 
> > > as
> > > proposed in
> > > https://patchwork.freedesktop.org/patch/526752/?series=109902=4 ? It
> > > gets updated if ioctl issuer is different than fd opener and this being
> > > context_init here reminded me of it. Maybe you wouldn't have to track the
> > > pid in msm?
>
> The problem is that we also need this for gpu devcore dumps, which
> could happen after the drm_file is closed.  The ctx can outlive the
> file.
>
I think we all kept forgetting about that. MSM had support for ages,
while AMDGPU is the second driver to land support - just a release
ago.

> But the ctx->pid has the same problem as the existing file->pid when
> it comes to Xorg.. hopefully over time that problem just goes away.

Out of curiosity: what do you mean with "when it comes to Xorg" - the
"was_master" handling or something else?

> guess I could do a similar dance to your patch to update the pid
> whenever (for ex) a submitqueue is created.
>
> > Can we go one step further and let the drm fdinfo stuff print these new
> > additions? Consistency across drivers and all that.
>
> Hmm, I guess I could _also_ store the overridden comm/cmdline in
> drm_file.  I still need to track it in ctx (msm_file_private) because
> I could need it after the file is closed.
>
> Maybe it could be useful to have a gl extension to let the app set a
> name on the context so that this is useful beyond native-ctx (ie.
> maybe it would be nice to see that "chrome: lwn.net" is using less gpu
> memory than "chrome: phoronix.com", etc)
>

/me awaits for the series to hit the respective websites ;-)

But seriously - the series from Tvrtko (thanks for the link, will
check in a moment) makes sense. Although given the livespan issue
mentioned above, I don't think it's applicable here.

So if it were me, I would consider the two orthogonal for the
short/mid term. Fwiw this and patch 1/3 are:
Reviewed-by: Emil Velikov 

HTH
-Emil

Re: [Freedreno] [RFC 2/2] drm/msm: Add memory stats to fdinfo

[Emil Velikov]

On Thu, 6 Apr 2023 at 22:59, Rob Clark  wrote:
>
> From: Rob Clark 
>
> Use the new helper to export stats about memory usage.
>
> Signed-off-by: Rob Clark 
> ---
>  drivers/gpu/drm/msm/msm_drv.c | 26 +-
>  drivers/gpu/drm/msm/msm_gpu.c |  2 --
>  2 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 9b6f17b1261f..385776f6a531 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1043,17 +1043,40 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
> DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query, 
> DRM_RENDER_ALLOW),
>  };
>
> +enum drm_gem_object_status gem_status(struct drm_gem_object *obj)
> +{
> +   struct msm_gem_object *msm_obj = to_msm_bo(obj);
> +   enum drm_gem_object_status status = 0;
> +
> +   if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true)))
> +   status |= DRM_GEM_OBJECT_ACTIVE;
> +
> +   if (msm_obj->pages)
> +   status |= DRM_GEM_OBJECT_RESIDENT;
> +
> +   if (msm_obj->madv == MSM_MADV_DONTNEED)
> +   status |= DRM_GEM_OBJECT_PURGEABLE;
> +
> +   return status;
> +}
> +
>  static void msm_fop_show_fdinfo(struct seq_file *m, struct file *f)
>  {
> struct drm_file *file = f->private_data;
> struct drm_device *dev = file->minor->dev;
> struct msm_drm_private *priv = dev->dev_private;
> +   struct msm_file_private *ctx = file->driver_priv;
> struct drm_printer p = drm_seq_file_printer(m);
>
> if (!priv->gpu)
> return;
>
> -   msm_gpu_show_fdinfo(priv->gpu, file->driver_priv, );
> +   drm_printf(, "drm-driver:\t%s\n", dev->driver->name);
> +   drm_printf(, "drm-client-id:\t%u\n", ctx->seqno);
> +
> +   msm_gpu_show_fdinfo(priv->gpu, ctx, );
> +
> +   drm_print_memory_stats(file, , gem_status);
>  }
>
>  static const struct file_operations fops = {
> @@ -1067,6 +1090,7 @@ static const struct drm_driver msm_driver = {
> DRIVER_RENDER |
> DRIVER_ATOMIC |
> DRIVER_MODESET |
> +   DRIVER_SYNCOBJ_TIMELINE |

This line should probably be its own patch. AFAICT it was supported
since ab723b7a992a19b843f798b183f53f7472f598c8, although explicitly
kept disabled until there's userspace/turnip support.

With the above line removed, the patch is:
Reviewed-by: Emil Velikov 

HTH
Emil

Re: [Freedreno] [RFC 1/2] drm: Add fdinfo memory stats

[Emil Velikov]

Hey Rob,

On Thu, 6 Apr 2023 at 22:59, Rob Clark  wrote:

> +- drm-purgeable-memory:  [KiB|MiB]
> +
> +The total size of buffers that are purgable.

s/purgable/purgeable/


> +static void print_size(struct drm_printer *p, const char *stat, size_t sz)
> +{
> +   const char *units[] = {"B", "KiB", "MiB", "GiB"};

The documentation says:

> Default unit shall be bytes with optional unit specifiers of 'KiB' or 'MiB'
> indicating kibi- or mebi-bytes.

So I would drop the B and/or update the documentation to mention B && GiB.

> +   unsigned u;
> +
> +   for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
> +   if (sz < SZ_1K)
> +   break;
> +   sz /= SZ_1K;

IIRC size_t can be 64bit, so we should probably use do_div() here.

> +   }
> +
> +   drm_printf(p, "%s:\t%lu %s\n", stat, sz, units[u]);
> +}
> +
> +/**
> + * drm_print_memory_stats - Helper to print standard fdinfo memory stats
> + * @file: the DRM file
> + * @p: the printer to print output to
> + * @status: callback to get driver tracked object status
> + *
> + * Helper to iterate over GEM objects with a handle allocated in the 
> specified
> + * file.  The optional status callback can return additional object state 
> which

s/return additional/return an additional/

> + * determines which stats the object is counted against.  The callback is 
> called
> + * under table_lock.  Racing against object status change is "harmless", and 
> the
> + * callback can expect to not race against object destroy.

s/destroy/destruction/

> + */
> +void drm_print_memory_stats(struct drm_file *file, struct drm_printer *p,
> +   enum drm_gem_object_status (*status)(struct 
> drm_gem_object *))
> +{

> +   if (s & DRM_GEM_OBJECT_RESIDENT) {
> +   size.resident += obj->size;
> +   s &= ~DRM_GEM_OBJECT_PURGEABLE;

Is MSM capable of marking the object as both purgeable and resident or
is this to catch other drivers? Should we add a note to the
documentation above - resident memory cannot be purgeable

> +   }
> +
> +   if (s & DRM_GEM_OBJECT_ACTIVE) {
> +   size.active += obj->size;
> +   s &= ~DRM_GEM_OBJECT_PURGEABLE;

Ditto.

With the above nits, the patch is:
Reviewed-by: Emil Velikov 

HTH
Emil

Re: [Freedreno] [v2] drm/msm: add shutdown support for display platform_driver

[Emil Velikov]

On Tue, 2 Jun 2020 at 17:10, Sai Prakash Ranjan
 wrote:
>
> Hi Emil,
>
> On 2020-06-02 21:09, Emil Velikov wrote:
> > On Tue, 2 Jun 2020 at 15:49, Sai Prakash Ranjan
> >  wrote:
> >>
> >> Hi Emil,
> >>
> >> On 2020-06-02 19:43, Emil Velikov wrote:
> >> > Hi Krishna,
> >> >
> >> > On Tue, 2 Jun 2020 at 08:17, Krishna Manikandan
> >> >  wrote:
> >> >>
> >> >> Define shutdown callback for display drm driver,
> >> >> so as to disable all the CRTCS when shutdown
> >> >> notification is received by the driver.
> >> >>
> >> >> This change will turn off the timing engine so
> >> >> that no display transactions are requested
> >> >> while mmu translations are getting disabled
> >> >> during reboot sequence.
> >> >>
> >> >> Signed-off-by: Krishna Manikandan 
> >> >>
> >> > AFAICT atomics is setup in msm_drm_ops::bind and shutdown in
> >> > msm_drm_ops::unbind.
> >> >
> >> > Are you saying that unbind never triggers? If so, then we should
> >> > really fix that instead, since this patch seems more like a
> >> > workaround.
> >> >
> >>
> >> Which path do you suppose that the unbind should be called from,
> >> remove
> >> callback? Here we are talking about the drivers which are builtin,
> >> where
> >> remove callbacks are not called from the driver core during
> >> reboot/shutdown,
> >> instead shutdown callbacks are called which needs to be defined in
> >> order
> >> to
> >> trigger unbind. So AFAICS there is nothing to be fixed.
> >>
> > Interesting - in drm effectively only drm panels implement .shutdown.
> > So my naive assumption was that .remove was used implicitly by core,
> > as part of the shutdown process. Yet that's not the case, so it seems
> > that many drivers could use some fixes.
> >
> > Then again, that's an existing problem which is irrelevant for msm.
> > -Emil
>
> To give more context, we are actually targeting the clients/consumers
> of SMMU/IOMMU here because we have to make sure that before the supplier
> (SMMU) shuts down, its consumers/clients need to be shutdown properly.
> Now the ordering of this is taken care in the SMMU driver via
> device_link
> which makes sure that consumer shutdown callbacks are called first, but
> we
> need to define shutdown callbacks for all its consumers to make sure we
> actually shutdown the clients or else it would invite the crashes during
> reboot
> which in this case was seen for display.
>
Thank you very much for the extra details. I think other DRM drivers
will be safe as-is.

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

Re: [Freedreno] [v2] drm/msm: add shutdown support for display platform_driver

[Emil Velikov]

On Tue, 2 Jun 2020 at 15:49, Sai Prakash Ranjan
 wrote:
>
> Hi Emil,
>
> On 2020-06-02 19:43, Emil Velikov wrote:
> > Hi Krishna,
> >
> > On Tue, 2 Jun 2020 at 08:17, Krishna Manikandan
> >  wrote:
> >>
> >> Define shutdown callback for display drm driver,
> >> so as to disable all the CRTCS when shutdown
> >> notification is received by the driver.
> >>
> >> This change will turn off the timing engine so
> >> that no display transactions are requested
> >> while mmu translations are getting disabled
> >> during reboot sequence.
> >>
> >> Signed-off-by: Krishna Manikandan 
> >>
> > AFAICT atomics is setup in msm_drm_ops::bind and shutdown in
> > msm_drm_ops::unbind.
> >
> > Are you saying that unbind never triggers? If so, then we should
> > really fix that instead, since this patch seems more like a
> > workaround.
> >
>
> Which path do you suppose that the unbind should be called from, remove
> callback? Here we are talking about the drivers which are builtin, where
> remove callbacks are not called from the driver core during
> reboot/shutdown,
> instead shutdown callbacks are called which needs to be defined in order
> to
> trigger unbind. So AFAICS there is nothing to be fixed.
>
Interesting - in drm effectively only drm panels implement .shutdown.
So my naive assumption was that .remove was used implicitly by core,
as part of the shutdown process. Yet that's not the case, so it seems
that many drivers could use some fixes.

Then again, that's an existing problem which is irrelevant for msm.
-Emil
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [v2] drm/msm: add shutdown support for display platform_driver

[Emil Velikov]

Hi Krishna,

On Tue, 2 Jun 2020 at 08:17, Krishna Manikandan  wrote:
>
> Define shutdown callback for display drm driver,
> so as to disable all the CRTCS when shutdown
> notification is received by the driver.
>
> This change will turn off the timing engine so
> that no display transactions are requested
> while mmu translations are getting disabled
> during reboot sequence.
>
> Signed-off-by: Krishna Manikandan 
>
AFAICT atomics is setup in msm_drm_ops::bind and shutdown in
msm_drm_ops::unbind.

Are you saying that unbind never triggers? If so, then we should
really fix that instead, since this patch seems more like a
workaround.

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

Re: [Freedreno] [PATCH 05/12] drm/msm/dpu: Stop copying around mode->private_flags

[Emil Velikov]

On Wed, 19 Feb 2020 at 20:36, Ville Syrjala
 wrote:
>
> From: Ville Syrjälä 
>
> The driver never sets mode->private_flags so copying
> it back and forth is entirely pointless. Stop doing it.
>
> Also drop private_flags from the tracepoint.
>
> Cc: Rob Clark 
> Cc: Sean Paul 
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä 

Perhaps the msm team has a WIP which makes use of it ?

Otherwise:
Reviewed-by: Emil Velikov 

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

Re: [Freedreno] [PATCH 07/13] drm/msm: drop DRM_AUTH usage from the driver

[Emil Velikov]

On Mon, 27 May 2019 at 09:19, Emil Velikov  wrote:
>
> From: Emil Velikov 
>
> The authentication can be circumvented, by design, by using the render
> node.
>
> From the driver POV there is no distinction between primary and render
> nodes, thus we can drop the token.
>
> Cc: Rob Clark 
> Cc: Sean Paul 
> Cc: freedreno@lists.freedesktop.org
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Signed-off-by: Emil Velikov 
> ---
>  drivers/gpu/drm/msm/msm_drv.c | 22 +++---
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 31deb87abfc6..ac1c7a8a85d0 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -983,17 +983,17 @@ static int msm_ioctl_submitqueue_close(struct 
> drm_device *dev, void *data,
>  }
>
>  static const struct drm_ioctl_desc msm_ioctls[] = {
> -   DRM_IOCTL_DEF_DRV(MSM_GET_PARAM,msm_ioctl_get_param,
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_GEM_NEW,  msm_ioctl_gem_new,  
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_GEM_INFO, msm_ioctl_gem_info, 
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_PREP, msm_ioctl_gem_cpu_prep, 
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_FINI, msm_ioctl_gem_cpu_fini, 
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_GEM_SUBMIT,   msm_ioctl_gem_submit,   
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_WAIT_FENCE,   msm_ioctl_wait_fence,   
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_GEM_MADVISE,  msm_ioctl_gem_madvise,  
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_NEW,   msm_ioctl_submitqueue_new,   
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_CLOSE, msm_ioctl_submitqueue_close, 
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query, 
> DRM_AUTH|DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_GET_PARAM,msm_ioctl_get_param,
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_GEM_NEW,  msm_ioctl_gem_new,  
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_GEM_INFO, msm_ioctl_gem_info, 
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_PREP, msm_ioctl_gem_cpu_prep, 
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_FINI, msm_ioctl_gem_cpu_fini, 
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_GEM_SUBMIT,   msm_ioctl_gem_submit,   
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_WAIT_FENCE,   msm_ioctl_wait_fence,   
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_GEM_MADVISE,  msm_ioctl_gem_madvise,  
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_NEW,   msm_ioctl_submitqueue_new,   
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_CLOSE, msm_ioctl_submitqueue_close, 
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query, 
> DRM_RENDER_ALLOW),
>  };
>
>  static const struct vm_operations_struct vm_ops = {
> --

Humble poke?

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

Re: [Freedreno] [PATCH v6 00/24] Associate ddc adapters with connectors

[Emil Velikov]

  |   7 +-
>  drivers/gpu/drm/imx/imx-tve.c |   6 +-
>  drivers/gpu/drm/mediatek/mtk_hdmi.c   |   7 +-
>  drivers/gpu/drm/mgag200/mgag200_mode.c|  13 +-
>  drivers/gpu/drm/msm/hdmi/hdmi_connector.c |   6 +-
drivers/gpu/drm/msm/edp/edp_ctrl.c:1
- not applicable: aux dp/mst

drivers/gpu/drm/nouveau/nouveau_connector.c:2
- should be updated at some point (as you pointed out).


drivers/gpu/drm/panel/panel-simple.c:1
- no applicable: panel driver


>  drivers/gpu/drm/radeon/radeon_connectors.c| 142 +-
>  drivers/gpu/drm/rockchip/inno_hdmi.c  |   6 +-
>  drivers/gpu/drm/rockchip/rk3066_hdmi.c|   7 +-
>  drivers/gpu/drm/sti/sti_hdmi.c|   6 +-
>  drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c|   7 +-
>  drivers/gpu/drm/tegra/hdmi.c  |   7 +-
>  drivers/gpu/drm/tegra/sor.c   |   7 +-

drivers/gpu/drm/tegra/output.c:1
- already handled in hdmi/sor

>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c|   6 +-
>  drivers/gpu/drm/vc4/vc4_hdmi.c    |  12 +-
>  drivers/gpu/drm/zte/zx_hdmi.c |   6 +-
>  drivers/gpu/drm/zte/zx_vga.c  |   6 +-
>  include/drm/drm_connector.h   |  18 +++
>  26 files changed, 336 insertions(+), 121 deletions(-)

In a Tl;Dr: I think this series covers 90%+ of the existing rather huge) 
driverset.

For the series:
Reviewed-by: Emil Velikov 

Fwiw I'm in favour of Jani's suggestion to fold the dcc into the usual
helper drm_connector_init(). Although since we have 130+ instances it
might be better left for another day.

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

Re: [Freedreno] [Patch v2 02/10] drm/msm: using dev_get_drvdata directly

[Emil Velikov]

On Thu, 4 Jul 2019 at 08:26, Fuqian Huang  wrote:
>
> Several drivers cast a struct device pointer to a struct
> platform_device pointer only to then call platform_get_drvdata().
> To improve readability, these constructs can be simplified
> by using dev_get_drvdata() directly.
>
> Signed-off-by: Fuqian Huang 

This patch is:
Reviewed-by: Emil Velikov 

I think you want to add Jordan's ack-by from [1]

-Emil
[1] https://lists.freedesktop.org/archives/dri-devel/2019-July/224928.html
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v3 00/22] Associate ddc adapters with connectors

[Emil Velikov]

On Fri, 28 Jun 2019 at 17:45, Daniel Vetter  wrote:
>
> On Fri, Jun 28, 2019 at 06:01:14PM +0200, Andrzej Pietrasiewicz wrote:
> > It is difficult for a user to know which of the i2c adapters is for which
> > drm connector. This series addresses this problem.
> >
> > The idea is to have a symbolic link in connector's sysfs directory, e.g.:
> >
> > ls -l /sys/class/drm/card0-HDMI-A-1/ddc
> > lrwxrwxrwx 1 root root 0 Jun 24 10:42 /sys/class/drm/card0-HDMI-A-1/ddc \
> >   -> ../../../../soc/1388.i2c/i2c-2
> >
> > The user then knows that their card0-HDMI-A-1 uses i2c-2 and can e.g. run
> > ddcutil:
> >
> > ddcutil -b 2 getvcp 0x10
> > VCP code 0x10 (Brightness): current value =90, max 
> > value =   100
> >
> > The first patch in the series adds struct i2c_adapter pointer to struct
> > drm_connector. If the field is used by a particular driver, then an
> > appropriate symbolic link is created by the generic code, which is also 
> > added
> > by this patch.
> >
> > The second patch is an example of how to convert a driver to this new 
> > scheme.
> >
> > v1..v2:
> >
> > - used fixed name "ddc" for the symbolic link in order to make it easy for
> > userspace to find the i2c adapter
> >
> > v2..v3:
> >
> > - converted as many drivers as possible.
> >
> > PATCHES 3/22-22/22 SHOULD BE CONSIDERED RFC!
>
> There's a lot more drivers than this I think (i915 is absent as an
> example, but there should be tons more). Why are those not possible?

While I fully agree there are more drivers, at the same time I wonder.
Is it a good idea to expect all of those to be fixed in one go and
block patches addressing 15+ drivers?

Personally I think it's reasonable to have this, alongside a TODO
entry for other drivers.

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

Re: [Freedreno] [PATCH 1/4] gpu: Use dev_get_drvdata()

[Emil Velikov]

On Mon, 1 Jul 2019 at 13:37, Emil Velikov  wrote:
>
> Hi Fuqian,
>
> On Mon, 1 Jul 2019 at 08:13, Fuqian Huang  wrote:
> >
> > Using dev_get_drvdata directly.
> >
> > Signed-off-by: Fuqian Huang 
> > ---
> >  drivers/gpu/drm/msm/adreno/adreno_device.c  |  6 ++
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 13 +
> >  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c|  6 ++
> >  drivers/gpu/drm/msm/dsi/dsi_host.c  |  6 ++
> >  drivers/gpu/drm/msm/msm_drv.c   |  3 +--
> >  drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 15 +--
> >  drivers/gpu/drm/panfrost/panfrost_device.c  |  6 ++
> As far as i can see the patch is spot on, thanks for that.
>
> Can you split this in three since it covers 3 separate drivers.

Quick grep for "platform_get_drvdata(to_platform_device" showed a few
instances through various drivers - exynos, msm, etc.
If you can address those with v2 that would be great.

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

Re: [Freedreno] [PATCH 1/4] gpu: Use dev_get_drvdata()

[Emil Velikov]

Hi Fuqian,

On Mon, 1 Jul 2019 at 08:13, Fuqian Huang  wrote:
>
> Using dev_get_drvdata directly.
>
> Signed-off-by: Fuqian Huang 
> ---
>  drivers/gpu/drm/msm/adreno/adreno_device.c  |  6 ++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 13 +
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c|  6 ++
>  drivers/gpu/drm/msm/dsi/dsi_host.c  |  6 ++
>  drivers/gpu/drm/msm/msm_drv.c   |  3 +--
>  drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 15 +--
>  drivers/gpu/drm/panfrost/panfrost_device.c  |  6 ++
As far as i can see the patch is spot on, thanks for that.

Can you split this in three since it covers 3 separate drivers.
For each one you can get a smaller CC list - patches with 20+ people
tend to get blocked :-(

We can pick the PANFROST entries from the get_maintainer.pl output,
and add them to the commit message.
Thus git send-email will parse the commit message and automatically CC
the people ;-)

Cc: Rob Herring  (supporter:ARM MALI PANFROST DRM DRIVER)
Cc: Tomeu Vizoso  (supporter:ARM MALI
PANFROST DRM DRIVER)
Cc: dri-de...@lists.freedesktop.org (open list:ARM MALI PANFROST DRM DRIVER)

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

Re: [Freedreno] [PATCH 06/59] drm/prime: Actually remove DRIVER_PRIME everywhere

[Emil Velikov]

On 2019/06/14, Daniel Vetter wrote:
> Split out to make the functional changes stick out more.
> 
Since this patch flew-by, as standalone one (intentionally or not) I'd
add, anything vaguely like:

"Core users of DRIVER_PRIME were removed from core with prior patches."

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

Re: [Freedreno] [PATCH 07/13] drm/msm: drop DRM_AUTH usage from the driver

[Emil Velikov]

On Mon, 27 May 2019 at 09:19, Emil Velikov  wrote:
>
> From: Emil Velikov 
>
> The authentication can be circumvented, by design, by using the render
> node.
>
> From the driver POV there is no distinction between primary and render
> nodes, thus we can drop the token.
>
> Cc: Rob Clark 
> Cc: Sean Paul 
> Cc: freedreno@lists.freedesktop.org
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Signed-off-by: Emil Velikov 
> ---
>  drivers/gpu/drm/msm/msm_drv.c | 22 +++---
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 31deb87abfc6..ac1c7a8a85d0 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -983,17 +983,17 @@ static int msm_ioctl_submitqueue_close(struct 
> drm_device *dev, void *data,
>  }
>
>  static const struct drm_ioctl_desc msm_ioctls[] = {
> -   DRM_IOCTL_DEF_DRV(MSM_GET_PARAM,msm_ioctl_get_param,
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_GEM_NEW,  msm_ioctl_gem_new,  
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_GEM_INFO, msm_ioctl_gem_info, 
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_PREP, msm_ioctl_gem_cpu_prep, 
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_FINI, msm_ioctl_gem_cpu_fini, 
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_GEM_SUBMIT,   msm_ioctl_gem_submit,   
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_WAIT_FENCE,   msm_ioctl_wait_fence,   
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_GEM_MADVISE,  msm_ioctl_gem_madvise,  
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_NEW,   msm_ioctl_submitqueue_new,   
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_CLOSE, msm_ioctl_submitqueue_close, 
> DRM_AUTH|DRM_RENDER_ALLOW),
> -   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query, 
> DRM_AUTH|DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_GET_PARAM,msm_ioctl_get_param,
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_GEM_NEW,  msm_ioctl_gem_new,  
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_GEM_INFO, msm_ioctl_gem_info, 
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_PREP, msm_ioctl_gem_cpu_prep, 
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_FINI, msm_ioctl_gem_cpu_fini, 
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_GEM_SUBMIT,   msm_ioctl_gem_submit,   
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_WAIT_FENCE,   msm_ioctl_wait_fence,   
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_GEM_MADVISE,  msm_ioctl_gem_madvise,  
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_NEW,   msm_ioctl_submitqueue_new,   
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_CLOSE, msm_ioctl_submitqueue_close, 
> DRM_RENDER_ALLOW),
> +   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query, 
> DRM_RENDER_ALLOW),
>  };
>
>  static const struct vm_operations_struct vm_ops = {
> --
> 2.21.0
>
Humble poke?

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

[Freedreno] [PATCH 07/13] drm/msm: drop DRM_AUTH usage from the driver

[Emil Velikov]

From: Emil Velikov 

The authentication can be circumvented, by design, by using the render
node.

From the driver POV there is no distinction between primary and render
nodes, thus we can drop the token.

Cc: Rob Clark 
Cc: Sean Paul 
Cc: freedreno@lists.freedesktop.org
Cc: David Airlie 
Cc: Daniel Vetter 
Signed-off-by: Emil Velikov 
---
 drivers/gpu/drm/msm/msm_drv.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 31deb87abfc6..ac1c7a8a85d0 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -983,17 +983,17 @@ static int msm_ioctl_submitqueue_close(struct drm_device 
*dev, void *data,
 }
 
 static const struct drm_ioctl_desc msm_ioctls[] = {
-   DRM_IOCTL_DEF_DRV(MSM_GET_PARAM,msm_ioctl_get_param,
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(MSM_GEM_NEW,  msm_ioctl_gem_new,  
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(MSM_GEM_INFO, msm_ioctl_gem_info, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_PREP, msm_ioctl_gem_cpu_prep, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_FINI, msm_ioctl_gem_cpu_fini, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(MSM_GEM_SUBMIT,   msm_ioctl_gem_submit,   
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(MSM_WAIT_FENCE,   msm_ioctl_wait_fence,   
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(MSM_GEM_MADVISE,  msm_ioctl_gem_madvise,  
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_NEW,   msm_ioctl_submitqueue_new,   
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_CLOSE, msm_ioctl_submitqueue_close, 
DRM_AUTH|DRM_RENDER_ALLOW),
-   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query, 
DRM_AUTH|DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(MSM_GET_PARAM,msm_ioctl_get_param,
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(MSM_GEM_NEW,  msm_ioctl_gem_new,  
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(MSM_GEM_INFO, msm_ioctl_gem_info, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_PREP, msm_ioctl_gem_cpu_prep, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(MSM_GEM_CPU_FINI, msm_ioctl_gem_cpu_fini, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(MSM_GEM_SUBMIT,   msm_ioctl_gem_submit,   
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(MSM_WAIT_FENCE,   msm_ioctl_wait_fence,   
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(MSM_GEM_MADVISE,  msm_ioctl_gem_madvise,  
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_NEW,   msm_ioctl_submitqueue_new,   
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_CLOSE, msm_ioctl_submitqueue_close, 
DRM_RENDER_ALLOW),
+   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query, 
DRM_RENDER_ALLOW),
 };
 
 static const struct vm_operations_struct vm_ops = {
-- 
2.21.0

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

Re: [Freedreno] [PATCH 01/26] drm/irq: Don't check for DRIVER_HAVE_IRQ in drm_irq_(un)install

[Emil Velikov]

On Thu, 24 Jan 2019 at 16:58, Daniel Vetter  wrote:
>
> If a non-legacy driver calls these it's valid to assume there is
> interrupt support. The flag is really only needed for legacy drivers.

... legacy drivers which issue the IRQ via the DRM_IOCTL_CONTROL legacy IOCTL.

At a later stage, we might as well add LEGACY to the name:
DRIVER_LEGACY_HAVE_IRQ?


>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  | 2 +-
>  drivers/gpu/drm/arm/hdlcd_drv.c  | 2 +-
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 2 +-
>  drivers/gpu/drm/drm_irq.c| 6 --
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c| 2 +-
>  drivers/gpu/drm/gma500/psb_drv.c | 2 +-
>  drivers/gpu/drm/i915/i915_drv.c  | 2 +-
>  drivers/gpu/drm/meson/meson_drv.c| 2 +-
>  drivers/gpu/drm/msm/msm_drv.c| 3 +--
>  drivers/gpu/drm/mxsfb/mxsfb_drv.c| 3 +--
>  drivers/gpu/drm/qxl/qxl_drv.c| 2 +-
>  drivers/gpu/drm/radeon/radeon_drv.c  | 2 +-
>  drivers/gpu/drm/shmobile/shmob_drm_drv.c | 2 +-
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c  | 2 +-
>  drivers/gpu/drm/vc4/vc4_drv.c| 1 -
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c  | 2 +-
>  drivers/staging/vboxvideo/vbox_drv.c | 2 +-

Local grep shows one more non-legacy entry in
drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c

With that file addressed and trivial comment additions, patches: 1, 2,
3 and 26 are
Reviewed-by: Emil Velikov 

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

Re: [Freedreno] [Mesa-dev] [RFC] freedreno: import libdrm_freedreno + redesign submit

[Emil Velikov]

On Tue, 30 Oct 2018 at 18:15, Rob Clark  wrote:
>
> On Tue, Oct 30, 2018 at 1:34 PM Emil Velikov  wrote:
> >
> > On Tue, 30 Oct 2018 at 17:19, Rob Clark  wrote:
> > > On Tue, Oct 30, 2018 at 11:27 AM Emil Velikov  
> > > wrote:
> >
> > > > > > NOTE: if bisecting a build error takes you hear, try a clean build.
> > > > > > There are a bunch of ways things can go wrong if you still have
> > > > > > libdrm_freedreno cflags.
> > > > >
> > > > > Good note!
> > > > > (and s/hear/here/)
> > > > >
> > > > Or to make the note disappear and minimise the chance to even getting
> > > > here you can try the following:
> > > >  - patch 1 - dummy copy, mention the sha used as base
> > > >  - patch 2/3/4 - wire up Autoconf/Android/meson
> > > >  - patch 5/... - polish (remove freedreno_drmif.h includes and others)
> > > >
> > >
> > > fwiw, I'm not planning to remove libdrm_freedreno any time soon,
> > > because (a) new libdrm vs old mesa combo, and (b) I do have some other
> > > small utils that use libdrm_freedreno.  I'm just planning to not
> > > change it.
> > >
> > Fair enough. I guess those tools will not benefit from the reduced CPU
> > cycled, to warrant a rewrite.
> >
> > I would greatly appreciate if you can split things as mentioned earlier 
> > though.
> > After all you've used the exact same approach to create the lot.
> >
>
> Too late on that, it is already merged.  I had considered trying to
> split up into (1) import, plus (2) redesign, but there was too much
> churn related to converting the code over to mesa utils / data
> structures / etc, so I gave up.
>
Well I'm slow - serves me right. The argument sound quite meh... as
said you've already followed that approach while preparing it.

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

Re: [Freedreno] [Mesa-dev] [RFC] freedreno: import libdrm_freedreno + redesign submit

[Emil Velikov]

On Tue, 30 Oct 2018 at 17:19, Rob Clark  wrote:
> On Tue, Oct 30, 2018 at 11:27 AM Emil Velikov  
> wrote:

> > > > NOTE: if bisecting a build error takes you hear, try a clean build.
> > > > There are a bunch of ways things can go wrong if you still have
> > > > libdrm_freedreno cflags.
> > >
> > > Good note!
> > > (and s/hear/here/)
> > >
> > Or to make the note disappear and minimise the chance to even getting
> > here you can try the following:
> >  - patch 1 - dummy copy, mention the sha used as base
> >  - patch 2/3/4 - wire up Autoconf/Android/meson
> >  - patch 5/... - polish (remove freedreno_drmif.h includes and others)
> >
>
> fwiw, I'm not planning to remove libdrm_freedreno any time soon,
> because (a) new libdrm vs old mesa combo, and (b) I do have some other
> small utils that use libdrm_freedreno.  I'm just planning to not
> change it.
>
Fair enough. I guess those tools will not benefit from the reduced CPU
cycled, to warrant a rewrite.

I would greatly appreciate if you can split things as mentioned earlier though.
After all you've used the exact same approach to create the lot.

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

Re: [Freedreno] [Mesa-dev] [RFC] freedreno: import libdrm_freedreno + redesign submit

[Emil Velikov]

On Thu, 25 Oct 2018 at 10:32, Eric Engestrom  wrote:
>
> On Tuesday, 2018-10-23 10:49:26 -0400, mesa-dev-boun...@lists.freedesktop.org 
> wrote:
> > In the pursuit of lowering driver overhead, it became clear that some
> > amount of redesign of how libdrm_freedreno constructs the submit ioctl
> > would be needed.  In particular, as the gallium driver is starting to
> > make heavier use of CP_SET_DRAW_STATE state groups/objects, the over-
> > head of tracking cmd buffers and relocs becomes too much.  And for
> > "streaming" state, which isn't ever reused (like uniform uploads) the
> > overhead of allocating/freeing ringbuffer[1] objects is too high.
> >
> > This redesign makes two main changes:
> >
> >  1) Introduces a fd_submit object for tracking bos and cmds table
> > for the submit ioctl, making ringbuffer objects more light-
> > weight.  This was previously done in the ringbuffer.  But we
> > have many ringbuffer instances involved in a submit (gmem +
> > draw + potentially 1000's of state-group rbs), and only need
> > a single bos and cmds table.  (Reloc table is still per-rb)
> >
> > The submit is also a convenient place for a slab allocator for
> > ringbuffer objects.  Other options would have required locking
> > because, while we can guarantee allocations will only happen on
> > a single thread, free's could happen either on the application
> > thread or the flush_queue thread.  With the slab allocator in
> > the submit object, any frees that happen on the flush_queue
> > thread happen after we know that the application thread is done
> > with the submit.
> >
> >  2) Introduce a new "softpin" msm_ringbuffer_sp implementation that
> > does not use relocs and only has cmds table entries for IB1 (ie.
> > the cmdstream buffers that kernel needs to CP_INDIRECT_BUFFER
> > to from the RB).  To do this properly will require some updates
> > on the kernel side, so whether you get the softpin or legacy
> > submit/ringbuffer implementation at runtime depends on your
> > kernel version.
> >
> > To make all these changes in libdrm would basically require adding a
> > libdrm_freedreno2, so this is a good point to just pull the libdrm code
> > into mesa.  Plus it allows for using mesa's hashtable, slab allocator,
> > etc.  And it lets us have asserts enabled for debug mesa buids but
> > omitted for release builds.  And it makes life easier if further API
> > changes become necessary.
> >
libdrm_freedreno made sense when there was more than one user. Since
xf86-video-freedreno is a dead end it's better to make your life
easier.

> > At this point I haven't tried to pull in the kgsl backend.  Although
> > I left the level of vfunc indirection which would make it possible
> > to have other backends.  (And this was convenient to keep to allow
> > for the "softpin" ringbuffer to coexist.)
> >
Does that code even work with the latest kernel kgsl code?
I thought wasn't compatible for years.

> > NOTE: if bisecting a build error takes you hear, try a clean build.
> > There are a bunch of ways things can go wrong if you still have
> > libdrm_freedreno cflags.
>
> Good note!
> (and s/hear/here/)
>
Or to make the note disappear and minimise the chance to even getting
here you can try the following:
 - patch 1 - dummy copy, mention the sha used as base
 - patch 2/3/4 - wire up Autoconf/Android/meson
 - patch 5/... - polish (remove freedreno_drmif.h includes and others)

It'll make it far easier to verity, read and provide meaningful review.

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

Re: [Freedreno] [PATCH 03/13] drm: Add drm_puts() to complement drm_printf()

[Emil Velikov]

Hi Jordan,

I might be a bit late for the party, so consider the following jfyi.

On 24 July 2018 at 17:33, Jordan Crouse  wrote:

> +void drm_puts(struct drm_printer *p, const char *str)
One could easily use the compiler to detect if drm_printf or drm_puts
should be used. See the trace_printk define in include/linux/kernel.h.

> +{
> +   if (p->puts)
> +   p->puts(p, str);
> +   else
> +   drm_printf(p, "%s", str);

From a quick look from the existing three printers (seq_file, info and
debug) only the first one is updated with this series.
I would imagine that updating the other two and dropping the
drm_printf() fallback is a good move.

Otherwise one could easily assume that they have a fast path when they do not.

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

[Freedreno] [PATCH v2] drm/msm: don't deref error pointer in the msm_fbdev_create error path

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

Currently the error pointer returned by msm_alloc_stolen_fb gets passed
to drm_framebuffer_remove. The latter handles only NULL pointers, thus
a nasty crash will occur.

Drop the unnecessary fail label and the associated checks - both err and
fb will be set at this stage.

Cc: Rob Clark <robdcl...@gmail.com>
Cc: linux-arm-...@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Cc: freedreno@lists.freedesktop.org
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
v2: really remove the fail label :-\
---
 drivers/gpu/drm/msm/msm_fbdev.c | 11 ++-
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
index c178563fcd4d..456622b46335 100644
--- a/drivers/gpu/drm/msm/msm_fbdev.c
+++ b/drivers/gpu/drm/msm/msm_fbdev.c
@@ -92,8 +92,7 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
 
if (IS_ERR(fb)) {
dev_err(dev->dev, "failed to allocate fb\n");
-   ret = PTR_ERR(fb);
-   goto fail;
+   return PTR_ERR(fb);
}
 
bo = msm_framebuffer_bo(fb, 0);
@@ -151,13 +150,7 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
 
 fail_unlock:
mutex_unlock(>struct_mutex);
-fail:
-
-   if (ret) {
-   if (fb)
-   drm_framebuffer_remove(fb);
-   }
-
+   drm_framebuffer_remove(fb);
return ret;
 }
 
-- 
2.16.0

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

Re: [Freedreno] [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Emil Velikov]

On 22 March 2018 at 18:03, Harry Wentland <harry.wentl...@amd.com> wrote:
> On 2018-03-22 01:54 PM, Emil Velikov wrote:
>> Hi Ville,
>>
>> On 22 March 2018 at 15:22, Ville Syrjala <ville.syrj...@linux.intel.com> 
>> wrote:
>>> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>>>
>>> I really just wanted to fix i915 to re-enable its planes afer load
>>> detection (a two line patch). This is what I actually ended up with
>>> after I ran into a framebuffer refcount leak with said two line patch.
>>>
>>> I've tested this on a few i915 boxes and so far it's looking
>>> good. Everything else is just compile tested.
>>>
>> Mostly thinking out loud:
>>
>> Wondering if one cannot somehow (re)move plane->fb/crtc altogether.
>> Otherwise drivers will reintroduce similar code, despite the WARNs and
>> beefy documentation :-\
>
> Wouldn't that require an atomic conversion of all remaining drivers?
>
That or maybe move into plane->legacy->{fb,crtc}. Feel free to swap
'legacy' with flashier name.

Hmm back in 2015 we had a GSoC that updated BOCHS and CIRRUS drivers,
but they never got merged.
Don't recall the details - from memory the conversion seemed fine, but
there was either shortage on review/other.

Might be worth reviving that... regardless it's getting a bit off-topic.
-Emil

[1] 
https://www.google-melange.com/archive/gsoc/2015/orgs/xorg/projects/johnhunter.html
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH 00/23] drm: Eliminate plane->fb/crtc usage for atomic drivers

[Emil Velikov]

Hi Ville,

On 22 March 2018 at 15:22, Ville Syrjala  wrote:
> From: Ville Syrjälä 
>
> I really just wanted to fix i915 to re-enable its planes afer load
> detection (a two line patch). This is what I actually ended up with
> after I ran into a framebuffer refcount leak with said two line patch.
>
> I've tested this on a few i915 boxes and so far it's looking
> good. Everything else is just compile tested.
>
Mostly thinking out loud:

Wondering if one cannot somehow (re)move plane->fb/crtc altogether.
Otherwise drivers will reintroduce similar code, despite the WARNs and
beefy documentation :-\

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

Re: [Freedreno] [PATCH] kgsl: advertise DRI2Info v4 for !NoKMS case

[Emil Velikov]

Small correction: the commit title should say NoKMS (no !).

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

[Freedreno] [PATCH] kgsl: advertise DRI2Info v4 for !NoKMS case

[Emil Velikov]

From: Emil Velikov <emil.veli...@collabora.com>

As mentioned in the commit message, Xserver has sufficient fallbacks
when the video driver does not implement ScheduleSwap, ScheduleWaitMSC
and GetMSC.

As such we can "fake it" and advertise v4, instead of the current v3.

Cc: freedreno@lists.freedesktop.org
Cc: Rob Clark <robdcl...@gmail.com>
Signed-off-by: Emil Velikov <emil.veli...@collabora.com>
---
Rob, hope that you have a a2xx + kgsl + shim setup around and you can
give this a try.
---
 src/msm-dri2.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/msm-dri2.c b/src/msm-dri2.c
index 6f60c4d..e9a681b 100644
--- a/src/msm-dri2.c
+++ b/src/msm-dri2.c
@@ -596,7 +596,16 @@ MSMDRI2ScreenInit(ScreenPtr pScreen)
 
if (pMsm->NoKMS) {
info.driverName  = "kgsl";
-   info.version = 3;
+/* Driver does not have KMS, so strictly speaking we should
+ * use v3 here.
+ *
+ * Although X server is smart enough to use appropriate
+ * fallbacks, regardless of the version exposed.
+ *
+ * Bump to v4, since that allows us to simplify other
+ * components such as Mesa.
+ */
+   info.version = 4;
} else {
info.driverName  = "msm";
info.version = 6;
-- 
2.13.3

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

Re: [Freedreno] [PATCH v3] drm: Add DRM_MODE_ROTATE_ and DRM_MODE_REFLECT_ to UAPI

[Emil Velikov]

Hi Rob,

On 18 May 2017 at 02:39, Robert Foss <robert.f...@collabora.com> wrote:
> Add DRM_MODE_ROTATE_ and DRM_MODE_REFLECT_ defines to the UAPI
> as a convenience.
>
> Ideally the DRM_ROTATE_ and DRM_REFLECT_ property ids are looked up
> through the atomic API, but realizing that userspace is likely to take
> shortcuts and assume that the enum values are what is sent over the
> wire.
>
> As a result these defines are provided purely as a convenience to
> userspace applications.
>
> Signed-off-by: Robert Foss <robert.f...@collabora.com>
> ---
> Changes since v2:
>  - Changed define prefix from DRM_MODE_PROP_ to DRM_MODE_
>  - Fix compilation errors
>  - Changed comment formatting
>  - Deduplicated comment lines
>  - Clarified DRM_MODE_PROP_REFLECT_ comment
>
> Changes since v1:
>  - Moved defines from drm.h to drm_mode.h
>  - Changed define prefix from DRM_ to DRM_MODE_PROP_
>  - Updated uses of the defines to the new prefix
>  - Removed include from drm_rect.c
>  - Stopped using the BIT() macro
>
Reviewed-by: Emil Velikov <emil.veli...@collabora.com>

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

Re: [Freedreno] [RESEND PATCH] drm/msm: adreno: fix build error without debugfs

[Emil Velikov]

On 13 March 2017 at 17:00, Rob Clark  wrote:
> On Mon, Mar 13, 2017 at 12:43 PM, Arnd Bergmann  wrote:
>> The newly added a5xx support fails to build when debugfs is diabled:
>>
>> drivers/gpu/drm/msm/adreno/a5xx_gpu.c:849:4: error: 'struct msm_gpu_funcs' 
>> has no member named 'show'
>> drivers/gpu/drm/msm/adreno/a5xx_gpu.c:849:11: error: 'a5xx_show' undeclared 
>> here (not in a function); did you mean 'a5xx_irq'?
>>
>> This adds a missing #ifdef.
>>
>> Fixes: b5f103ab98c7 ("drm/msm: gpu: Add A5XX target support")
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Arnd Bergmann 
>
> I thought I had picked this one up already, but I guess my brain was
> playing tricks on me.. I've pushed this to msm-next for now and will
> cherry-pick this over to a -fixes branch when I send fixes for 4.11..
>
Sean sent a similar patch last week. I think he picked it (alongside
others) in drm-misc-fixes.
Haven't checked though ;-)

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

Re: [Freedreno] [PATCH 3/3] drm/msm: Fix compilation error when CONFIG_DEBUG_FS undefined

[Emil Velikov]

On 6 March 2017 at 20:27, Sean Paul  wrote:
> Fixes the following compilation error when CONFIG_DEBUG_FS undefined:
>
> CC [M]  drivers/gpu/drm/msm/adreno/a5xx_gpu.o
> ../drivers/gpu/drm/msm/adreno/a5xx_gpu.c:863:3: error: unknown field ‘show’ 
> specified in initializer
> ../drivers/gpu/drm/msm/adreno/a5xx_gpu.c:863:11: error: ‘a5xx_show’ 
> undeclared here (not in a function)
> ../drivers/gpu/drm/msm/adreno/a5xx_gpu.c:863:3: warning: excess elements in 
> struct initializer [enabled by default]
> ../drivers/gpu/drm/msm/adreno/a5xx_gpu.c:863:3: warning: (near initialization 
> for ‘funcs.base’) [enabled by default]
> make[5]: *** [drivers/gpu/drm/msm/adreno/a5xx_gpu.o] Error 1
> make[4]: *** [drivers/gpu/drm/msm] Error 2
> make[4]: *** Waiting for unfinished jobs
>
> Fixes: b5f103ab98c7 ("drm/msm: gpu: Add A5XX target support")
> Cc: Jordan Crouse 
> Cc: Rob Clark 
> Signed-off-by: Sean Paul 
> ---
>  drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c 
> b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index 4414cf73735d..f0c8bd74ca91 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -860,7 +860,9 @@ static const struct adreno_gpu_funcs funcs = {
> .idle = a5xx_idle,
> .irq = a5xx_irq,
> .destroy = a5xx_destroy,
> +#ifdef CONFIG_DEBUG_FS
> .show = a5xx_show,
> +#endif
A drm_compat_ioctl-like solution might be cleaner but this also works ;-)

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

Re: [Freedreno] [PATCH 03/11] drm/msm: Add hint to DRM_IOCTL_MSM_GEM_INFO to return an object IOVA

[Emil Velikov]

Hi Jordan,

On 6 February 2017 at 17:39, Jordan Crouse  wrote:
> Modify the 'pad' member of struct drm_msm_gem_info to 'hint'. If the
> user sets 'hint' to non-zero it means that they want a IOVA for the
> GEM object instead of a mmap() offset. Return the iova in the 'offset'
> member.
>
> Signed-off-by: Jordan Crouse 
> ---
>  drivers/gpu/drm/msm/msm_drv.c | 29 +
>  include/uapi/drm/msm_drm.h|  4 ++--
>  2 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index e29bb66..1e4e022 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -677,6 +677,17 @@ static int msm_ioctl_gem_cpu_fini(struct drm_device 
> *dev, void *data,
> return ret;
>  }
>
> +static int msm_ioctl_gem_info_iova(struct drm_device *dev,
> +   struct drm_gem_object *obj, uint64_t *iova)
> +{
> +   struct msm_drm_private *priv = dev->dev_private;
> +
> +   if (!priv->gpu)
> +   return -EINVAL;
> +
Not too familiar with msm so perhaps a silly question: how can we trigger this ?

> +   return msm_gem_get_iova(obj, priv->gpu->aspace, iova);
> +}
> +
>  static int msm_ioctl_gem_info(struct drm_device *dev, void *data,
> struct drm_file *file)
>  {
> @@ -684,14 +695,24 @@ static int msm_ioctl_gem_info(struct drm_device *dev, 
> void *data,
> struct drm_gem_object *obj;
> int ret = 0;
>
> -   if (args->pad)
> -   return -EINVAL;
> -
Please keep the input validation before doing any work (the lookup below).

> obj = drm_gem_object_lookup(file, args->handle);
> if (!obj)
> return -ENOENT;
>
> -   args->offset = msm_gem_mmap_offset(obj);
> +   /*
> +* If the hint variable is set, it means that the user wants a IOVA 
> for
> +* this buffer.  Return the address from the GPU because that is
> +* probably what it is looking for
> +*/
> +   if (args->hint) {
One could also rename hint to flags. Regardless of the name you can
use hint/flags as a bitmask.

> +   uint64_t iova;
> +
> +   ret = msm_ioctl_gem_info_iova(dev, obj, );
> +   if (!ret)
> +   args->offset = iova;
> +   } else {
> +   args->offset = msm_gem_mmap_offset(obj);
> +   }
>
> drm_gem_object_unreference_unlocked(obj);
>
> diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
> index 4d5d6a2..045ad20 100644
> --- a/include/uapi/drm/msm_drm.h
> +++ b/include/uapi/drm/msm_drm.h
> @@ -105,8 +105,8 @@ struct drm_msm_gem_new {
>
>  struct drm_msm_gem_info {
> __u32 handle; /* in */
> -   __u32 pad;
> -   __u64 offset; /* out, offset to pass to mmap() */
> +   __u32 hint;   /* in */
Please add explicit #define for the currently valid hints/flags.

> +   __u64 offset; /* out, mmap() offset if hint is 0, iova if 1 */
Other drivers have used anonymous unions to improve the naming, in
such situations.

struct drm_msm_gem_info {
__u32 handle;   /* in */
__u32 hint; /* in */
union { /* out */
  __u64 offset; /* offset if hint is FOO */
  __u64 iova;   /* iova if hint is BAR */
};
};


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

Re: [Freedreno] [PATCH] drm/msm: update uapi header license

[Emil Velikov]

On 19 October 2016 at 23:04, Rob Clark <robdcl...@gmail.com> wrote:
> The same file in libdrm is, as is the tradition with the rest of libdrm,
> etc, using an MIT license.  To avoid complications in the future with
> sync'ing the uapi header to libdrm, lets fix the license mismatch now
> before there are any non-trivial commits from someone other than myself.
>
Glad to see us (almost) there :-)

Acked-by: Emil Velikov <emil.l.veli...@gmail.com>

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