Re: [PATCH 3/4] drm/radeon: consolidate uvd/vce initialization, resume and suspend.

["Christian König" <christian.koenig@amd.com>]

Am 16.03.2016 um 15:59 schrieb Jerome Glisse:
> On Wed, Mar 16, 2016 at 2:03 PM, Christian König
> <deathsimple@vodafone.de> wrote:
>> Am 16.03.2016 um 13:48 schrieb Jérôme Glisse:
>>> From: Jérome Glisse <jglisse@redhat.com>
>>>
>>> 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@redhat.com>
>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>
>> 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.

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.

Regards,
Christian.

>
> Jérôme

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