From: "Christian König" <deathsimple@vodafone.de>
To: Jerome Glisse <j.glisse@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 02/10] drm/radeon: UVD bringup v7
Date: Wed, 03 Apr 2013 17:53:55 +0200 [thread overview]
Message-ID: <515C5093.9030709@vodafone.de> (raw)
In-Reply-To: <20130403145323.GC2010@gmail.com>
Am 03.04.2013 16:53, schrieb Jerome Glisse:
> On Wed, Apr 03, 2013 at 01:18:31AM +0200, Christian König wrote:
>> [SNIP]
>>
>> /* hardcode those limit for now */
>> #define RADEON_VA_IB_OFFSET (1 << 20)
>> #define RADEON_VA_RESERVED_SIZE (8 << 20)
>> @@ -357,8 +360,9 @@ struct radeon_bo_list {
>> struct ttm_validate_buffer tv;
>> struct radeon_bo *bo;
>> uint64_t gpu_offset;
>> - unsigned rdomain;
>> - unsigned wdomain;
>> + bool written;
>> + unsigned domain;
>> + unsigned alt_domain;
>> u32 tiling_flags;
>> };
> I think that the change to the rdomain/wdomain should be in a patch
> of its own. I think the change is fine but we had issue with change
> that touched that part previously, would make bisecting and
> understanding the change implication easier.
Agree, I actually planed to do so, but for the whole IP review stuff we
needed to maintain a more or less stable patch base. Long story, but I'm
going to change it.
>>
>> @@ -826,7 +830,6 @@ struct radeon_cs_reloc {
>> struct radeon_bo *robj;
>> struct radeon_bo_list lobj;
>> uint32_t handle;
>> - uint32_t flags;
>> };
> Why removing the flags ? iirc it's not really use right now but i
> remember plan to use it.
Ups, just a rebasing artifact. But when it's unused we should remove it,
probably just not in this patch.
>>
>> [SNIP]
>>
>> +static int radeon_uvd_cs_reloc(struct radeon_cs_parser *p, int data0, int data1)
>> +{
>> + struct radeon_cs_chunk *relocs_chunk;
>> + struct radeon_cs_reloc *reloc;
>> + unsigned idx, cmd;
>> + uint64_t start, end;
>> +
>> + relocs_chunk = &p->chunks[p->chunk_relocs_idx];
>> + idx = radeon_get_ib_value(p, data1);
>> + if (idx >= relocs_chunk->length_dw) {
>> + DRM_ERROR("Relocs at %d after relocations chunk end %d !\n",
>> + idx, relocs_chunk->length_dw);
>> + return -EINVAL;
>> + }
>> +
>> + reloc = p->relocs_ptr[(idx / 4)];
>> + start = reloc->lobj.gpu_offset;
>> + end = start + radeon_bo_size(reloc->robj);
>> + start += radeon_get_ib_value(p, data0);
> I am assuming there is no way for you to know the size that the uvd engine will write to ?
> You are not checking anything on uvd possibly overwritting after the bo end.
Yeah that gave me headache for a quite long time, too. The problem is to
figure out how much is actually written you need to keep track of the
whole lot of informations including the UVD session,
create/decode/destroy messages and allot of fiddling with the codec
specific parameters.
And if I understand the UVD internals correctly even if we check
everything there is no guarantee that a special crafted bitstream could
not let UVD to write over the end of the buffer....
Is it ok if we but a big TODO on it for the initial patch?
Cheers,
Christian.
next prev parent reply other threads:[~2013-04-03 15:53 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-02 23:18 [PATCH] drm/radeon: UVD support for RV710-SI Christian König
2013-04-02 23:18 ` [PATCH 01/10] drm/radeon: UVD doesn't needs VM on SI Christian König
2013-04-03 14:42 ` Jerome Glisse
2013-04-03 15:29 ` Christian König
2013-04-02 23:18 ` [PATCH 02/10] drm/radeon: UVD bringup v7 Christian König
2013-04-03 14:53 ` Jerome Glisse
2013-04-03 15:53 ` Christian König [this message]
2013-04-03 17:10 ` Jerome Glisse
2013-04-04 13:26 ` Christian König
2013-04-04 16:57 ` Alex Deucher
2013-04-02 23:18 ` [PATCH 03/10] drm/radeon: add pm callback for setting uvd clocks Christian König
2013-04-02 23:18 ` [PATCH 04/10] drm/radeon: add radeon_atom_get_clock_dividers helper Christian König
2013-04-02 23:18 ` [PATCH 05/10] drm/radeon: add set_uvd_clocks callback for ON/LN/TN (v4) Christian König
2013-04-02 23:18 ` [PATCH 06/10] drm/radeon: add set_uvd_clocks callback for evergreen Christian König
2013-04-02 23:18 ` [PATCH 07/10] drm/radeon: add set_uvd_clocks callback for SI Christian König
2013-04-02 23:18 ` [PATCH 08/10] drm/radeon: add set_uvd_clocks callback for r7xx v3 Christian König
2013-04-02 23:18 ` [PATCH 09/10] drm/radeon: init UVD clocks to sane defaults Christian König
2013-04-02 23:18 ` [PATCH 10/10] drm/radeon: add UVD tiling addr config v2 Christian König
2013-04-02 23:34 ` [PATCH] drm/radeon: UVD support for RV710-SI Alex Deucher
2013-04-03 14:41 ` Jerome Glisse
2013-04-03 17:57 ` Andreas Boll
2013-04-04 8:41 ` Christian König
2013-04-04 12:06 ` Paul Menzel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=515C5093.9030709@vodafone.de \
--to=deathsimple@vodafone.de \
--cc=dri-devel@lists.freedesktop.org \
--cc=j.glisse@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.