On Wed, Mar 16, 2016 at 04:19:16PM +0100, Christian König wrote:
> Am 16.03.2016 um 15:59 schrieb Jerome Glisse:
> >On Wed, Mar 16, 2016 at 2:03 PM, Christian König
> ><deathsimple@xxxxxxxxxxx> wrote:
> >>Am 16.03.2016 um 13:48 schrieb Jérôme Glisse:
> >>>From: Jérome Glisse <jglisse@xxxxxxxxxx>
> >>>
> >>>This consolidate uvd/vce into a common shape for all generation. It
> >>>also leverage the rdev->has_uvd flags to know what it is useless to
> >>>try to resume/suspend uvd/vce block.
> >>>
> >>>There is no functional changes when there is no error. On error the
> >>>device driver will behave differently than before after this patch.
> >>>It should now safely ignore uvd/vce errors and keeps normal operation
> >>>of others engine. This is an improvement over current situation where
> >>>we have different behavior depending on GPU generation and on what
> >>>fails.
> >>>
> >>>Finaly this is a preparatory step for a patch which allow to disable
> >>>uvd/vce as a driver option.
> >>>
> >>>This have only been tested on southern island so please test it on
> >>>other generations (i do not have hardware handy for now).
> >>>
> >>>Signed-off-by: Jérôme Glisse <jglisse@xxxxxxxxxx>
> >>>Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> >>>Cc: Christian König <christian.koenig@xxxxxxx>
> >>
> >>NAK, skipping UVD and VCE suspend/resume when initialization fails should
> >>already be implemented.
> >>
> >>There might be some (quite some) bugs in there, but that doesn't justify
> >>reworking the initialization over all different generations. Especially
> >>since you don't have hardware to test all of them.
> >>
> >>Just make sure that radeon_uvd/vce_fini() is called when something goes
> >>wrong and/or that the UVD/VCE BO is properly released.
> >>
> >>Regards,
> >>Christian.
> >Current code is a mess when it comes to handling error related to
> >uvd/vce. This patch consolidate control flow in something easy to
> >follow. You can check that there is absolulety no control flow change
> >for the case where uvd/vce works and thus it does not break anything
> >that works. It will only gracefully fails and cleanup if things go
> >wrong. So while i have not tested on other hw i am confident that this
> >does not introduce regression.
> >
> >I tried to do it without consolidation but it ended up in adding even
> >more if() levels that line did begins after 80colums. So please
> >reconsider because this is an improvement over existing code.
>
> Well then please point out at the example of the SI or CIK code what exactly
> is missing here.
Going from :
if (rdev->has_uvd) {
r = uvd_v2_2_resume(rdev);
if (!r) {
r = radeon_fence_driver_start_ring(rdev,
R600_RING_TYPE_UVD_INDEX);
if (r)
dev_err(rdev->dev, "UVD fences init error (%d).\n", r);
}
if (r)
rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
}
r = radeon_vce_resume(rdev);
if (!r) {
r = vce_v1_0_resume(rdev);
if (!r)
r = radeon_fence_driver_start_ring(rdev,
TN_RING_TYPE_VCE1_INDEX);
if (!r)
r = radeon_fence_driver_start_ring(rdev,
TN_RING_TYPE_VCE2_INDEX);
}
if (r) {
dev_err(rdev->dev, "VCE init error (%d).\n", r);
rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
}
To:
r = uvd_v2_2_resume(rdev);
if (r)
goto error;
r = radeon_fence_driver_start_ring(rdev, R600_RING_TYPE_UVD_INDEX);
if (r)
goto error_uvd;
r = radeon_vce_resume(rdev);
if (r)
goto error_uvd;
r = vce_v1_0_resume(rdev);
if (r)
goto error_vce;
r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE1_INDEX);
if (r)
goto error_vce;
r = radeon_fence_driver_start_ring(rdev, TN_RING_TYPE_VCE2_INDEX);
if (r)
goto error_vce;
return;
error_vce:
radeon_vce_suspend(rdev);
error_uvd:
radeon_uvd_suspend(rdev);
error:
dev_err(rdev->dev, "UVD/VCE startup error (%d).\n", r);
/* On error just disable everything. */
radeon_vce_fini(rdev);
radeon_uvd_fini(rdev);
rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
Is lot more clear to me than bunch of intertwine if/else. A clear error path
for which you do not have to jump through if level to see what get executed
or not on error. The only difference is that it does tie uvd and vce together.
I did that on purpose because on the hw i am playing with the vce seems to be
useless when the uvd block fails (opposite seems to be true too). If you think
we should still try to init vce when uvd fails or uvd when vce fails i can
split uvd and vce.
The other difference with existing code is that i free resources normaly use
uvd/vce on error (free fw buffer). This is just me trying to free resource
early and it has no impact as block are not working.
-----------------------------------------------------------------------------------
Second part we go from:
if (rdev->has_uvd) {
ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
if (ring->ring_size) {
r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
RADEON_CP_PACKET2);
if (!r)
r = uvd_v1_0_init(rdev);
if (r)
DRM_ERROR("radeon: failed initializing UVD (%d).\n", r);
}
}
r = -ENOENT;
ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
if (ring->ring_size)
r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
VCE_CMD_NO_OP);
ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
if (ring->ring_size)
r = radeon_ring_init(rdev, ring, ring->ring_size, 0,
VCE_CMD_NO_OP);
if (!r)
r = vce_v1_0_init(rdev);
else if (r != -ENOENT)
DRM_ERROR("radeon: failed initializing VCE (%d).\n", r);
To:
ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
r = radeon_ring_init(rdev, ring, ring->ring_size, 0, RADEON_CP_PACKET2);
if (r)
goto error;
r = uvd_v1_0_init(rdev);
if (r)
goto error_uvd;
ring = &rdev->ring[TN_RING_TYPE_VCE1_INDEX];
r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
if (r)
goto error_vce1;
ring = &rdev->ring[TN_RING_TYPE_VCE2_INDEX];
r = radeon_ring_init(rdev, ring, ring->ring_size, 0, VCE_CMD_NO_OP);
if (r)
goto error_vce2;
r = vce_v1_0_init(rdev);
if (r)
goto error_vce;
return;
error_vce:
radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE2_INDEX]);
error_vce2:
radeon_ring_fini(rdev, &rdev->ring[TN_RING_TYPE_VCE1_INDEX]);
error_vce1:
uvd_v1_0_fini(rdev);
error_uvd:
radeon_ring_fini(rdev, &rdev->ring[R600_RING_TYPE_UVD_INDEX]);
error:
dev_err(rdev->dev, "UVD/VCE resume error (%d).\n", r);
/* On error just disable everything. */
radeon_uvd_suspend(rdev);
radeon_vce_suspend(rdev);
radeon_uvd_fini(rdev);
radeon_vce_fini(rdev);
rdev->ring[R600_RING_TYPE_UVD_INDEX].ring_size = 0;
rdev->ring[TN_RING_TYPE_VCE1_INDEX].ring_size = 0;
rdev->ring[TN_RING_TYPE_VCE2_INDEX].ring_size = 0;
Again lot simpler to follow control flow than to jump through various level
of if/else. Again uvd and vce tied together (and again i can untie them if
you think it is better to untie them).
But this time the extra thing is that i properly disable ring if any error
happens while existing code does not.
I am not pasting the init path but it the same logic, tying uvd and vce
together and simplifying error code path.
>
> Please also note that VCE/UVD has dependencies on power management, so that
> when they are once initialized they should NOT be turned off again.
>
> I only briefly skimmed over your patch, but it actually looks like to me
> that you broken that by trying to cleanup the initialization routine.
I have seen that but assuming Heisenbergs does not get involve, then given
that the block is not responding to register write it is unlikely that thing
will we worse if we try to disable the block. And from my testing it does
not impact power management. My guess is that the block keep reporting it is
busy and that power gating and clock gating are inhibited by that.
The other thing i am doing over existing code is freeing memory for the fw
buffer. I do not think it is a big deal. I am doing that because then i just
flag the uvd has dead (rdev->has_uvd = 0) and avoid to try to restore it
for next suspend/resume cycle or hibernation cycle.
So again the only thing i am change is the case where thing does not work.
With that patch i can actualy hibernate laptop and get back a working desktop
module video decoding/encoding no longer working. I call that an improvement.
Jérôme
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
