Hi Daniel,
On Fri, Jun 14, 2019 at 04:53:08PM +0800, james qian wang (Arm Technology
China) wrote:
> On Fri, Jun 14, 2019 at 09:01:11AM +0200, Daniel Vetter wrote:
> > On Fri, Jun 14, 2019 at 05:46:04AM +0000, james qian wang (Arm Technology
> > China) wrote:
> > > On Thu, Jun 13, 2019 at 04:30:08PM +0200, Daniel Vetter wrote:
> > > > On Thu, Jun 13, 2019 at 02:24:37PM +0100, Liviu Dudau wrote:
> > > > > On Thu, Jun 13, 2019 at 11:08:14AM +0200, Daniel Vetter wrote:
> > > > > > On Thu, Jun 13, 2019 at 09:28:13AM +0100, Liviu Dudau wrote:
> > > > > > > On Thu, Jun 13, 2019 at 10:17:27AM +0200, Daniel Vetter wrote:
> > > > > > > > On Wed, Jun 12, 2019 at 02:26:24AM +0000, james qian wang (Arm
> > > > > > > > Technology China) wrote:
> > > > > > > > > On Tue, Jun 11, 2019 at 02:30:38PM +0200, Daniel Vetter wrote:
> > > > > > > > > > On Tue, Jun 11, 2019 at 11:13:45AM +0000, Lowry Li (Arm
> > > > > > > > > > Technology China) wrote:
> > > > > > > > > > > From: "Lowry Li (Arm Technology China)" <lowry...@arm.com>
> > > > > > > > > > >
> > > > > > > > > > > The komeda internal resources (pipelines) are shared
> > > > > > > > > > > between crtcs,
> > > > > > > > > > > and resources release by disable_crtc. This commit is
> > > > > > > > > > > working for once
> > > > > > > > > > > user forgot disabling crtc like app quit abnomally, and
> > > > > > > > > > > then the
> > > > > > > > > > > resources can not be used by another crtc. Adds
> > > > > > > > > > > drop_master to
> > > > > > > > > > > shutdown the device and make sure all the komeda
> > > > > > > > > > > resources have been
> > > > > > > > > > > released and can be used for the next usage.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Lowry Li (Arm Technology China)
> > > > > > > > > > > <lowry...@arm.com>
> > > > > > > > > > > ---
> > > > > > > > > > > drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 13
> > > > > > > > > > > +++++++++++++
> > > > > > > > > > > 1 file changed, 13 insertions(+)
> > > > > > > > > > >
> > > > > > > > > > > diff --git
> > > > > > > > > > > a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > > > > > b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > > > > > index 8543860..647bce5 100644
> > > > > > > > > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > > > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> > > > > > > > > > > @@ -54,11 +54,24 @@ static irqreturn_t
> > > > > > > > > > > komeda_kms_irq_handler(int irq, void *data)
> > > > > > > > > > > return status;
> > > > > > > > > > > }
> > > > > > > > > > >
> > > > > > > > > > > +/* Komeda internal resources (pipelines) are shared
> > > > > > > > > > > between crtcs, and resources
> > > > > > > > > > > + * are released by disable_crtc. But if user forget
> > > > > > > > > > > disabling crtc like app quit
> > > > > > > > > > > + * abnormally, the resources can not be used by another
> > > > > > > > > > > crtc.
> > > > > > > > > > > + * Use drop_master to shutdown the device and make sure
> > > > > > > > > > > all the komeda resources
> > > > > > > > > > > + * have been released, and can be used for the next
> > > > > > > > > > > usage.
> > > > > > > > > > > + */
> > > > > > > > > >
> > > > > > > > > > No. If we want this, we need to implement this across
> > > > > > > > > > drivers, not with
> > > > > > > > > > per-vendor hacks.
> > > > > > > > > >
> > > > > > > > > > The kerneldoc should have been a solid hint: "Only used by
> > > > > > > > > > vmwgfx."
> > > > > > > > > > -Daniel
> > > > > > > > >
> > > > > > > > > Hi Daniel:
> > > > > > > > > This drop_master is really what we want, can we update the
> > > > > > > > > doc and
> > > > > > > > > add komeda as a user of this hacks like "used by vmwfgx and
> > > > > > > > > komeda",
> > > > > > > > > or maybe directly promote this per-vendor hacks as an
> > > > > > > > > optional chip
> > > > > > > > > function ?
> > > > > > > >
> > > > > > > > Still no, because it would mean different behaviour for
> > > > > > > > arm/komeda
> > > > > > > > compared to everyone else. And we really don't want this,
> > > > > > > > because this
> > > > > > > > would completely break flicker-less vt-switching.
> > > > > > > >
> > > > > > > > Currently the only fallback for this case is the lastclose
> > > > > > > > handler, which
> > > > > > > > atm just restores fbcon/fbdev. If you want to change/extend
> > > > > > > > that to work
> > > > > > > > without fbdev, then that's the place to do the change. And
> > > > > > > > across _all_
> > > > > > > > drm kms drivers, so that we have consistent behaviour.
> > > > > > >
> > > > > > > Slightly unrelated, just thinking of a solution and wanted
> > > > > > > confirmation/double
> > > > > > > checking: can a CRTC be instantiated without any planes (or
> > > > > > > without a primary
> > > > > > > plane)?
> > > > > >
> > > > > > Without a primary plane maybe not so recommended, because it would
> > > > > > break
> > > > > > all the legacy userspace. Might even result in some oopses, not
> > > > > > sure we
> > > > > > check for crtc->primary != NULL.
> > > > > >
> > > > > > I'm not sure what you mean about instantiating it without any plane
> > > > > > at
> > > > > > all. That would be rather useless.
> > > > >
> > > > > Agree, and I think I have one way of solving the scenario Lowry and
> > > > > James are
> > > > > trying to cover. Basically, komeda has 2 pipelines that are exposed
> > > > > as 2 crtcs.
> > > > > However, layers (planes in DRM) can be associated with any of the
> > > > > pipelines and
> > > > > it is possible to have a DRM master open up crtc0 and enable all
> > > > > possible planes,
> > > > > which would leave crtc1 with no available layer to use (but
> > > > > technically still visible to
> > > > > userspace, as it has been drm_crtc_init-ed. James and Lowry are
> > > > > trying to give
> > > > > another master a chance of enabling crtc1 if previous master drops the
> > > > > ownership of crtc0 without disabling it. So one solution I'm thinking
> > > > > of is to
> > > > > tie one of the layers/planes to crtc1 regardless if that pipeline is
> > > > > enabled or
> > > > > not.
> > > > >
> > > > > Alternatively, we need a more generic solution for re-allocating
> > > > > resources
> > > > > between CRTCs that might be enabled at different times. Ideas on how
> > > > > userspace
> > > > > should handle it first are welcome as well.
> > > >
> > > > Uh, you can't have more than one active master. And that other master
> > > > needs to clean up the mess left behind by the previous one. Generally
> > > > that
> > >
> > > Yes komeda needs such clean up (disableing all planes is
> > > enough for such cleanup), but no matter it handles by old master or
> > > the new master, just before the new master enable the crtc is enouth
> > > for komeda.
> > > I think the problem here is which place we should do such cleanup:
> > > USER or KERNEL mode.
> > >
> > > For kernel: set_master/drop_master is a good place.
> > > For user: enter_vt and leave_vt.
> >
> > Well someone else already made that decision for you, userspace needs to
> > clean up.
> >
> > If we want to change that, then we need to have a subsystem wide
> > discussion, and roll out the new behaviour (without breaking any
> > expectations of current userspace, which your patch does) for everyone.
> >
> > For more context, I've written up what could all be possible new/existing
> > solutions in this space quite a while ago:
> >
> > https://blog.ffwll.ch/2016/01/vt-switching-with-atomic-modeset.html
> >
> > > Why I decide to add it to kernel:
> > > - So many users: X/wayland/Android, avoid duplidated code.
> > > - we also plan to support third-part user. they may not do such
> > > cleanup for komeda.
> >
> > Fix your userspace :-)
> >
>
> Hi Daniel:
> Thank you, it's very good article, very clear for why we adopt
> user-mode to do save/restore for VT-switching.
>
> I think I should follow.
>
> Actually our komeda user has such logic already. :)
>
> Hi Lowry:
>
> Per Daniel, we should drop this patch.
>
> Thanks
> James
Hi Daniel,
Thanks a lot for this. I will send a new patchset to drop this patch.
Regards,
Lowry
> > Cheers, Daniel
> >
> > >
> > > Thanks
> > > James
> > >
> > > > means disabling all the planes, like e.g. fbdev emulation does (but
> > > > there's a lot more we should probably clean up, current code is kinda
> > > > just
> > > > good enough).
> > > >
> > > > If you want multiple concurrent masters on the same hw, then you need
> > > > drm
> > > > leases, and there you can limit the leases to however many planes you
> > > > think they need.
> > > > -Daniel
> > > >
> > > > >
> > > > > Best regards,
> > > > > Liviu
> > > > >
> > > > > > -Daniel
> > > > > >
> > > > > > >
> > > > > > > Best regards,
> > > > > > > Liviu
> > > > > > >
> > > > > > > >
> > > > > > > > kms is a cross-vendor api, vendor hacks are very, very much not
> > > > > > > > cool.
> > > > > > > > -Daniel
> > > > > > > >
> > > > > > > > >
> > > > > > > > > James
> > > > > > > > >
> > > > > > > > > > > +static void komeda_kms_drop_master(struct drm_device
> > > > > > > > > > > *dev,
> > > > > > > > > > > + struct drm_file *file_priv)
> > > > > > > > > > > +{
> > > > > > > > > > > +drm_atomic_helper_shutdown(dev);
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > static struct drm_driver komeda_kms_driver = {
> > > > > > > > > > > .driver_features = DRIVER_GEM | DRIVER_MODESET |
> > > > > > > > > > > DRIVER_ATOMIC |
> > > > > > > > > > > DRIVER_PRIME | DRIVER_HAVE_IRQ,
> > > > > > > > > > > .lastclose= drm_fb_helper_lastclose,
> > > > > > > > > > > .irq_handler= komeda_kms_irq_handler,
> > > > > > > > > > > +.master_drop= komeda_kms_drop_master,
> > > > > > > > > > > .gem_free_object_unlocked= drm_gem_cma_free_object,
> > > > > > > > > > > .gem_vm_ops= &drm_gem_cma_vm_ops,
> > > > > > > > > > > .dumb_create= komeda_gem_cma_dumb_create,
> > > > > > > > > > > --
> > > > > > > > > > > 1.9.1
> > > > > > > > > > >
> > > > > > > > > > > _______________________________________________
> > > > > > > > > > > dri-devel mailing list
> > > > > > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > Daniel Vetter
> > > > > > > > > > Software Engineer, Intel Corporation
> > > > > > > > > > http://blog.ffwll.ch
> > > > > > > > > IMPORTANT NOTICE: The contents of this email and any
> > > > > > > > > attachments are confidential and may also be privileged. If
> > > > > > > > > you are not the intended recipient, please notify the sender
> > > > > > > > > immediately and do not disclose the contents to any other
> > > > > > > > > person, use it for any purpose, or store or copy the
> > > > > > > > > information in any medium. Thank you.
> > > > > > > > > _______________________________________________
> > > > > > > > > dri-devel mailing list
> > > > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > > > >
> > > > > > > > --
> > > > > > > > Daniel Vetter
> > > > > > > > Software Engineer, Intel Corporation
> > > > > > > > http://blog.ffwll.ch
> > > > > > >
> > > > > > > --
> > > > > > > ====================
> > > > > > > | I would like to |
> > > > > > > | fix the world, |
> > > > > > > | but they're not |
> > > > > > > | giving me the |
> > > > > > > \ source code! /
> > > > > > > ---------------
> > > > > > > ¯\_(ツ)_/¯
> > > > > > > _______________________________________________
> > > > > > > dri-devel mailing list
> > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > >
> > > > > > --
> > > > > > Daniel Vetter
> > > > > > Software Engineer, Intel Corporation
> > > > > > http://blog.ffwll.ch
> > > > >
> > > > > --
> > > > > ====================
> > > > > | I would like to |
> > > > > | fix the world, |
> > > > > | but they're not |
> > > > > | giving me the |
> > > > > \ source code! /
> > > > > ---------------
> > > > > ¯\_(ツ)_/¯
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > >
> > > --
> > > Reviewed-by: James Qian Wang (Arm Technology China)
> > > <james.qian.w...@arm.com>
> > > IMPORTANT NOTICE: The contents of this email and any attachments are
> > > confidential and may also be privileged. If you are not the intended
> > > recipient, please notify the sender immediately and do not disclose the
> > > contents to any other person, use it for any purpose, or store or copy
> > > the information in any medium. Thank you.
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Reviewed-by: James Qian Wang (Arm Technology China) <james.qian.w...@arm.com>
--
Regards,
Lowry
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
