Re: [PATCHv5,RESEND 3/8] gpu: host1x: Add channel support

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1: Type: text/plain, Size: 1848 bytes --]

On Fri, Mar 08, 2013 at 06:16:16PM +0200, Terje Bergström wrote:
> On 26.02.2013 11:48, Terje Bergström wrote:
> > On 25.02.2013 17:24, Thierry Reding wrote:
[...]
> >>> +struct mem_handle;
> >>> +struct platform_device;
> >>> +
> >>> +struct host1x_job_unpin_data {
> >>> +     struct mem_handle *h;
> >>> +     struct sg_table *mem;
> >>> +};
> >>> +
> >>> +enum mem_mgr_flag {
> >>> +     mem_mgr_flag_uncacheable = 0,
> >>> +     mem_mgr_flag_write_combine = 1,
> >>> +};
> >>
> >> I'd like to see this use a more object-oriented approach and more common
> >> terminology. All of these handles are essentially buffer objects, so
> >> maybe something like host1x_bo would be a nice and short name.
> 
> We did this a bit differently, but following pretty much the same
> principles. We have host1x_mem_handle, which contains an ops pointer.
> The handle gets encapsulated inside drm_gem_cma_object.
> 
> _bo structs seem to usually contains a drm_gem_object, so we thought
> it's better not to reuse that term.
> 
> Please check the code and let us know what you think. This pretty much
> follows what Lucas proposed a while ago, and keeps neatly the DRM
> specific parts inside the drm directory.

A bo is just a buffer object, so I don't see why the name shouldn't be
used. The name is in no way specific to DRM or GEM. But the point that I
was trying to make was that there is nothing to suggest that we couldn't
use drm_gem_object as the underlying scaffold to base all host1x buffer
objects on.

Furthermore I don't understand why you've chosen this approach. It is
completely different from what other drivers do and therefore makes it
more difficult to comprehend. That alone I could live with if there were
any advantages to that approach, but as far as I can tell there are
none.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]