From mboxrd@z Thu Jan 1 00:00:00 1970 From: "C, Ramalingam" Subject: Re: [PATCH v8 05/35] drm/i915: MEI interface definition Date: Thu, 13 Dec 2018 09:25:25 +0530 Message-ID: <3a96dcfd-fd36-157a-1f67-3d2250160713@intel.com> References: <1543315413-24302-1-git-send-email-ramalingam.c@intel.com> <1543315413-24302-6-git-send-email-ramalingam.c@intel.com> <20181206102303.GO21184@phenom.ffwll.local> <20181207142950.GE21184@phenom.ffwll.local> <20181212103802.GU21184@phenom.ffwll.local> <89a723a1-6649-4b93-8b3d-004cb99350a7@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0603660478==" Return-path: In-Reply-To: <89a723a1-6649-4b93-8b3d-004cb99350a7@intel.com> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: daniel.vetter@ffwll.ch, intel-gfx@lists.freedesktop.org, uma.shankar@intel.com, tomas.winkler@intel.com, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org This is a multi-part message in MIME format. --===============0603660478== Content-Type: multipart/alternative; boundary="------------188DC68DA188B8CEBB18F017" Content-Language: en-US This is a multi-part message in MIME format. --------------188DC68DA188B8CEBB18F017 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit On 12/12/2018 4:34 PM, C, Ramalingam wrote: > > > On 12/12/2018 4:08 PM, Daniel Vetter wrote: >> On Wed, Dec 12, 2018 at 02:28:29PM +0530, C, Ramalingam wrote: >>> On 12/7/2018 7:59 PM, Daniel Vetter wrote: >>>> On Fri, Dec 07, 2018 at 11:22:44AM +0530, C, Ramalingam wrote: >>>>> On 12/6/2018 3:53 PM, Daniel Vetter wrote: >>>>>> On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote: >>>>>>> +static void i915_hdcp_component_master_unbind(struct device *dev) >>>>>>> +{ >>>>>>> + struct drm_i915_private *dev_priv = kdev_to_i915(dev); >>>>>>> + >>>>>>> + intel_connectors_hdcp_disable(dev_priv); >>>>>> Why this code? Once we've unregistered the driver, we should have shut >>>>>> down the hardware completely. Which should have shut down all the hdcp >>>>>> users too. >>>>> This unbind might be triggered either due to master_del or component_del. >>>>> if its triggered from mei through component_del, then before teardown of >>>>> the i/f in component_unbind(), disable the ongoing HDCP session through >>>>> hdcp_disable() for each connectors. >>>> I looked at your v7 component code again. I think if we put the >>>> drm_atomic_helper_shutdown() call into master_unbind, then we'll have taken care >>>> of that. Essentially what you're doing here is open-coding part of that >>>> function. Better to just move the existing call to the right place. >>>> >>>> And shutting down the entire hw is kinda what master_unbind should be >>>> doing I think. We might also need to move the general hw quiescent into >>>> master_unbind, that should work too. >>> Need some more information on this. As per v7 on master_unbind will invoke >>> i915_unload_head that is i915_driver_unregister(dev_priv); >>> Similarly master_bind will call the load_tail that is i915_driver_register(dev_priv); >>> >>> We are not initializing/shutting the HW in master_bind/unbind . >>> But this comment is contradicting with current approach. Could you please elaborate.? >> So my understanding is that we need to shut down all hdcp usage before >> master_unbind completes, because otherwise we'll blow up: The mei_hdcp >> component is gone already. >> >> Now the other case for master_unbind is that the i915 pci device is >> unloaded, and in that case again I think it makes sense to shut down the >> hardware in master_unbind. >> >> Now when I looked at v7, right after the component_unbind code in the i915 >> unload sequences comes the function calls to shut down the hardware. I >> think it would make sense to them (or at least the >> drm_atomic_helper_shutdown() call) into master_unbind. >> >> This is somewhat asymetric, but that's ok: Load path doesn't enable >> anything, we just keep the hardware as-is (to be able to support >> flicker-free boôt-up). First modest is done by usersapce. Exception is if >> you have fbcon enabled (and not use the fastboot patches that Hans just >> merged), in that case the kernel will do the first modeset when we >> regiseter the fbdev device. >> >> Anyway, moving the drm_atomic_helper_shutdown() into the master_unbind >> function in v7 should take care of disabling all outputs, and hence also >> disabling all usage of hdcp component. > But in that case master_bind should do the reverse of the drm_atomic_helper_shutdown()right? > else lets say mei_hdcp is removed, master_unbind triggered and hence all hw is shutdown. > And then mei_hdcp is loaded so master_bind should initialize the hw right? Which is not the scenario right now. Summarizing the #intel-gfx IRC discussion: As i915 load and unload  are already asymetric, there is no harm in moving the drm_atomic_helper_shutdown() into the master_unbind(). So going ahead with daniel's suggestion. -Ram > > -Ram >> -Daniel --------------188DC68DA188B8CEBB18F017 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit


On 12/12/2018 4:34 PM, C, Ramalingam wrote:


On 12/12/2018 4:08 PM, Daniel Vetter wrote:
On Wed, Dec 12, 2018 at 02:28:29PM +0530, C, Ramalingam wrote:

On 12/7/2018 7:59 PM, Daniel Vetter wrote:

On Fri, Dec 07, 2018 at 11:22:44AM +0530, C, Ramalingam wrote:

On 12/6/2018 3:53 PM, Daniel Vetter wrote:

On Tue, Nov 27, 2018 at 04:13:03PM +0530, Ramalingam C wrote:

+static void i915_hdcp_component_master_unbind(struct device *dev)
+{
+	struct drm_i915_private *dev_priv = kdev_to_i915(dev);
+
+	intel_connectors_hdcp_disable(dev_priv);

Why this code? Once we've unregistered the driver, we should have shut
down the hardware completely. Which should have shut down all the hdcp
users too.

This unbind might be triggered either due to master_del or component_del.
if its triggered from mei through component_del, then before teardown of
the i/f in component_unbind(), disable the ongoing HDCP session through
hdcp_disable() for each connectors.

I looked at your v7 component code again. I think if we put the
drm_atomic_helper_shutdown() call into master_unbind, then we'll have taken care
of that. Essentially what you're doing here is open-coding part of that
function. Better to just move the existing call to the right place.

And shutting down the entire hw is kinda what master_unbind should be
doing I think. We might also need to move the general hw quiescent into
master_unbind, that should work too.

Need some more information on this. As per v7 on master_unbind will invoke
i915_unload_head that is i915_driver_unregister(dev_priv);
Similarly master_bind will call the load_tail that is i915_driver_register(dev_priv);

We are not initializing/shutting the HW in master_bind/unbind .
But this comment is contradicting with current approach. Could you please elaborate.?

So my understanding is that we need to shut down all hdcp usage before
master_unbind completes, because otherwise we'll blow up: The mei_hdcp
component is gone already.

Now the other case for master_unbind is that the i915 pci device is
unloaded, and in that case again I think it makes sense to shut down the
hardware in master_unbind.

Now when I looked at v7, right after the component_unbind code in the i915
unload sequences comes the function calls to shut down the hardware. I
think it would make sense to them (or at least the
drm_atomic_helper_shutdown() call) into master_unbind.

This is somewhat asymetric, but that's ok: Load path doesn't enable
anything, we just keep the hardware as-is (to be able to support
flicker-free boôt-up). First modest is done by usersapce. Exception is if
you have fbcon enabled (and not use the fastboot patches that Hans just
merged), in that case the kernel will do the first modeset when we
regiseter the fbdev device.

Anyway, moving the drm_atomic_helper_shutdown() into the master_unbind
function in v7 should take care of disabling all outputs, and hence also
disabling all usage of hdcp component.
But in that case master_bind should do the reverse of the drm_atomic_helper_shutdown()right?
else lets say mei_hdcp is removed, master_unbind triggered and hence all hw is shutdown.
And then mei_hdcp is loaded so master_bind should initialize the hw right? Which is not the scenario right now.
Summarizing the #intel-gfx IRC discussion:
As i915 load and unload  are already asymetric, there is no harm in moving
the drm_atomic_helper_shutdown() into the master_unbind(). So going
ahead with daniel's suggestion.

-Ram


-Ram

-Daniel
--------------188DC68DA188B8CEBB18F017-- --===============0603660478== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0603660478==--