This small set of patches adds support for the host1x and a subset of
the display controller hardware available on NVIDIA Tegra SoCs.
The first patch makes the RGB output available, which is directly driven
by its parent display controller and usually connected to an LVDS bridge
in embedded and notebook applications.
The second patch adds support for the HDMI output which can be driven by
any of the two display controllers.
This set of patches uses the GEM/CMA and KMS/FB helpers by Sascha Hauer
and Lars-Peter Clausen respectively.
Note that the driver is fully functional, but a few things are still
missing from this series because they depend on other patches that have
not been included in mainline yet. One such series is Steffen Trumtrar's
display helper series that allows the display modes to be defined within
the device tree, which comes in handy for embedded applications. What's
also missing from this series is the glue to wire up a backlight device
with a DRM connector so that the backlight can be switched on and off at
the proper time. I will submit incremental patches as the dependencies
make it into Linus' tree.
There is a full set of patches available in the tegra/next branch of my
repository on gitorious[0]. I know that a few people have already tested
the code on that branch, which has been very helpful in ironing out some
of the final details.
I fully realize that this is awfully late, but I still hope to get this
in for 3.8. I've talked about this with David Airlie and he said if I
can get the patches reviewed sometime before 3.7-rc6 or 3.7-rc7 by some
people of the embedded DRM crowd he may take them into the 3.8 merge
window. That would leave about 2 weeks for review, so if anybody could
find the time to look at this code that'd be great.
During the development of this series I've received a lot of feedback
and many helpful suggestions from the people at NVIDIA, so I owe them
big thanks.
Thierry
[0]: git://gitorious.org/thierryreding/linux.git
Thierry Reding (2):
drm: Add NVIDIA Tegra20 support
drm: tegra: Add HDMI support
This commit adds a KMS driver for the Tegra20 SoC. This includes basic
support for host1x and the two display controllers found on the Tegra20
SoC. Each display controller can drive a separate RGB/LVDS output.
This commit adds support for the HDMI output on the Tegra20 SoC. Only
one such output is available, but it can be driven by either of the two
display controllers.
A lot of work on this patch has been contributed by NVIDIA's Mark Zhang
<ma...@nvidia.com> and many other people at NVIDIA were very helpful in
getting the HDMI support and surrounding infrastructure to work.
<thierry.red...@avionic-design.de> wrote:
> This commit adds a KMS driver for the Tegra20 SoC. This includes basic
> support for host1x and the two display controllers found on the Tegra20
> SoC. Each display controller can drive a separate RGB/LVDS output.
Yeah, that's indeed true. And honestly adding just another implementation of the HDMI info frames sounds like somebody should finally sit down and implement it in a common drm_hdmi.c
> Yeah, that's indeed true. And honestly adding just another implementation of
> the HDMI info frames sounds like somebody should finally sit down and
> implement it in a common drm_hdmi.c
>> note that drm already has it's own debugfs helpers, see
>> drm_debugfs_create_files() and drm_debugfs_remove_files()
>> And also see debugfs_init/debugfs_cleanup in 'struct drm_driver'.
>> You probably want to be using that rather than rolling your own. You
>> can have a look at omapdrm for a quite simple example, or i915 for a
>> more complex example.
> I actually tried to make use of those functions, but unfortunately it's
> not possible to hook it up properly. The reason is that I need to pass
> in some reference to the tegra_dc structure in order to read the
> registers, but the DRM debugfs support doesn't allow to do that. Or
> maybe it can. There's the void *arg argument that I could possibly use.
> Then again it won't allow the creation of separate directories for each
> of the display controllers. Or maybe I'm missing something.
yeah, no separate directories.. but you could use the void *arg. It
is a bit awkward for dealing with multiple subdevices, we have the
same issue w/ omapdrm where dmm is a separate subdevice (and
dsi/dpi/hdmi/etc too shortly, as we merge omapdss and omapdrm).
But I guess better handling in drm for subdevices would help a lot of
the SoC platforms. Maybe something that I'll give some more thought
later after the atomic pageflip/modeset stuff is sorted.
>> maybe these should be in dc.h? Seems like these are related to the dc hw block?
> Yes, they could go into dc.h. This is one of the things that is likely
> to change at some point as more of the host1x support is added, which is
> where those syncpts are actually used.
hmm, are these values defined by the hw? They look like register
offsets into the DC block?
>> btw, if I understand the register/unregister client stuff, I think
>> there can be some potential issues. If I understand correctly, you
>> will create the crtc's in register. But unregister doesn't destroy
>> them, drm_mode_config_cleanup() when the container drm device is
>> unloaded does. So if there is any possibility to unregister a client
>> without tearing down everything, you might get into some problems
>> here.
>> Also, you might be breaking some assumptions about when the crtc's are
>> created.. at least if there is some possibility for userspace to sneak
>> in and do a getresources ioctl between the first and second client.
>> That might be easy enough to solve by adding some event for userspace
>> to get notified that it should getresources again. The unregister is
>> what I worry about more.
>> In general drm isn't set up to well for drivers that are a collection
>> of sub-devices. It is probably worth trying to improve this, although
>> I am still a bit skittish about the edge cases, like what happens when
>> you unload a sub-device mid-modeset. The issue comes up again for
>> common panel/display framework.
>> But for now you might just want to do something to ensure that all the
>> sub-devices are loaded/unloaded together as a whole.
> The way that the above is supposed to work is that the DRM relevant
> host1x clients are kept in a separate list and only if all of them have
> successfully been probed is the DRM portion initialized. Upon
> unregistration, as soon as the first of these clients is unregistered,
> all of the DRM stuff is torn down.
ahh, ok, I guess if DRM is torn down on first unregister, then you
shouldn't be hitting issues. I wasn't sure if the intention was to be
able to load/unload clients independently (such as building them as
separate modules eventually)
BR,
-R
> I don't believe there's an issue here. It's precisely what I've been
> testing for a while, always making sure that when built as a module it
> can properly be unloaded.
> That said it probably won't matter very much since on Tegra all drivers
> are usually builtin, so none of this may even be used in the end.
> This commit adds a KMS driver for the Tegra20 SoC. This includes basic
> support for host1x and the two display controllers found on the Tegra20
> SoC. Each display controller can drive a separate RGB/LVDS output.
> diff --git a/Documentation/devicetree/bindings/gpu/drm/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/gpu/drm/nvidia,tegra20-host1x.txt
> new file mode 100644
> index 0000000..b4fa934
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/drm/nvidia,tegra20-host1x.txt
"drm" is a Linux-specific term, so shouldn't really be used as the
directory name for a binding. bindings/gpu/nvidia,tegra20-host1x.txt
would probably be just fine.
Aside from that, the bindings,
Acked-by: Stephen Warren <swar...@nvidia.com>
I don't really know anything about DRM or our display HW, so I haven't
reviewed the code at all. I certainly ack the concept of adding the
driver though! I have asked various other people at NVIDIA to give a
quick review of the code.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> This commit adds a KMS driver for the Tegra20 SoC. This includes basic
> support for host1x and the two display controllers found on the Tegra20
> SoC. Each display controller can drive a separate RGB/LVDS output.
I applied these two patches and the two arch/arm/mach-tegra patches you
posted, directly on top of next-20121109, and I see the following build
failure:
> drivers/gpu/drm/tegra/output.c: In function 'tegra_output_init':
> drivers/gpu/drm/tegra/output.c:166:9: error: 'struct tegra_output' has no member named 'display'
> drivers/gpu/drm/tegra/output.c:166:3: error: implicit declaration of function 'of_get_display'
> drivers/gpu/drm/tegra/output.c:167:20: error: 'struct tegra_output' has no member named 'display'
> drivers/gpu/drm/tegra/output.c:168:25: error: 'struct tegra_output' has no member named 'display'
> drivers/gpu/drm/tegra/output.c:179:13: error: 'struct tegra_output' has no member named 'display'
> drivers/gpu/drm/tegra/output.c:180:3: error: implicit declaration of function 'display_put'
> drivers/gpu/drm/tegra/output.c:180:21: error: 'struct tegra_output' has no member named 'display'
> drivers/gpu/drm/tegra/output.c:257:20: error: 'struct tegra_output' has no member named 'display'
> drivers/gpu/drm/tegra/output.c: In function 'tegra_output_exit':
> drivers/gpu/drm/tegra/output.c:272:20: error: 'struct tegra_output' has no member named 'display'
Does this depend on something not in linux-next?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> On Fri, Nov 09, 2012 at 10:26:27AM -0600, Rob Clark wrote:
>> hmm, are these values defined by the hw? They look like register >> offsets into the DC block? > I don't think they are defined by the hardware. From what I gather these
> can arbitrarily be assigned by software. If things actually work the way
> I think they do, then eventually these values could be allocated by the
> host1x_register_client() function and stored within the host1x_client
> structure, so that each HW block can program them into the corresponding
> register.
These are host1x sync points. Sync points are used to synchronize work between host1x, host1x client units (like DC, 2D, EPP, etc), and CPU. Tegra2 TRM now contains chapters for HOST1X, 2D and EPP, so it has some more details.
The assignment of sync points is a software policy. Depending on programming model of client unit, one or more sync points are used for each. For example, for each DC we have one sync point assigned to vblank, and one for each DC window. For 2D, we'd have one sync point, and a choice of using the same of different sync point for EPP.
We could either assign sync point registers by hard coding, or assign them dynamically one per client unit, and possibly an additional one depending on the programming model. Sync points are a scarce resource, so we've so far preferred to do static assignment to catch overallocation as early as possible.
> > Yeah, that's indeed true. And honestly adding just another
> > implementation of the HDMI info frames sounds like somebody should
> > finally sit down and implement it in a common drm_hdmi.c
> So I've been looking at what most other implementations do and it seems
> a lot just fill the AVI infoframe with zeroes while only a few actually
> try to put useful information in them. Still in order to plan for a
> generic solution, I thought maybe something like the below set of
> structures and functions could work:
> /*
> * Structure that contains the infoframe fields in a form that allows them to
> * be easily accessed from C code.
> */
> struct hdmi_avi_infoframe;
> /*
> * DRM helper to fill a struct hdmi_avi_infoframe with information taken from
> * a struct drm_display_mode. Fields that cannot automatically be derived by
> * looking at a struct drm_display_mode are set to the default values.
> */
> int drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
> struct drm_display_mode *mode);
> /*
> * Packs the struct hdmi_avi_infoframe into a binary buffer that can be
> * programmed to the hardware-specific registers.
> */
> ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe *frame,
> void *buffer, size_t size);
> Such a scheme would allow DRM drivers to call the helper and tweak the
> fields in the structure if the want or need to and call the packing
> function to obtain a buffer that they can write to the controller.
> Does that sound at all reasonable?
Sounds good, especially the disdinction between the infoframe creation and
packing. E.g. on intel sdvo outputs we may not put in one of the ECC bytes
(since the hw creates it), so we need our own packing code there.
-Daniel
-- Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch --
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
<thierry.red...@avionic-design.de> wrote:
> Actually what I had in mind was a packed binary representation of
> infoframes as specified by HDMI 1.3a (I don't have access to 1.4, but I
> would think it doesn't differ in this respect) in section 5.3 and 5.3.5
> more specifically. According to the specification, the ECC bytes only
> come into play at a later stage, when data is actually transmitted on
> the TMDS link (Section 5.2.3). Tegra, nouveau and radeon also seem to be
> doing the checksumming in hardware, so I guess we don't need to compute
> the ECC bytes in software at all (for now).
> Once we have this for the AVI infoframes I guess the same concept can be
> used for audio infoframes and for vendor-specific infoframes (for HDMI
> 1.4 3D).
Iirc there's more than one checksum: The ECC field at byte 3 and the
checksum field at byte 4. All intel hw computes the ECC itself, but
some want us to store the infoframe with an empty ECC byte as
placeholder, whereas others (sdvo encoders) insert that byte
themselves, i.e. the infoframe is actually one byte shorter. In any
case, that kind of mangling can be done in the driver with easy.
-Daniel
-- Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch --
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
This second version of this patch series addresses all the comments
received so far. Most notably it takes advantage of the debugfs helpers
provided by the DRM core. Oddly enough this actually increases the line
count, but that's because the helpers don't fit with the subdevices
approach as implemented by this driver. However some quick discussions
with Rob Clark showed that Tegra DRM is not special in this respect but
other drivers may need the same functionality. Eventually the debugfs
code could be reworked on top of helpers that are better suited at the
design of embedded, multi-device DRM drivers.
Other than that there is some removal of code that was actually supposed
to go into a later patch because it has dependencies that haven't been
merged yet and some moving around of #defines and the device tree
bindings documentation. Finally the driver now uses the DRM core's
drm_compat_ioctl() instead of a custom and unimplemented (!) version.
Thierry
Thierry Reding (2):
drm: Add NVIDIA Tegra20 support
drm: tegra: Add HDMI support
This commit adds support for the HDMI output on the Tegra20 SoC. Only
one such output is available, but it can be driven by either of the two
display controllers.
A lot of work on this patch has been contributed by NVIDIA's Mark Zhang
<ma...@nvidia.com> and many other people at NVIDIA were very helpful in
getting the HDMI support and surrounding infrastructure to work.
Signed-off-by: Thierry Reding <thierry.red...@avionic-design.de>
---
Changes in v2:
- reuse debugfs infrastructure provided by the DRM core
This commit adds a KMS driver for the Tegra20 SoC. This includes basic
support for host1x and the two display controllers found on the Tegra20
SoC. Each display controller can drive a separate RGB/LVDS output.
Signed-off-by: Thierry Reding <thierry.red...@avionic-design.de>
---
Changes in v2:
- drop Linux-specific drm subdirectory for DT bindings documentation
- remove display helper leftovers that belong in a later patch
- reuse debugfs infrastructure provided by the DRM core
- move vblank syncpoint defines to dc.h
- use drm_compat_ioctl()
> This second version of this patch series addresses all the comments
> received so far. Most notably it takes advantage of the debugfs helpers
> provided by the DRM core. Oddly enough this actually increases the line
> count, but that's because the helpers don't fit with the subdevices
> approach as implemented by this driver. However some quick discussions
> with Rob Clark showed that Tegra DRM is not special in this respect but
> other drivers may need the same functionality. Eventually the debugfs
> code could be reworked on top of helpers that are better suited at the
> design of embedded, multi-device DRM drivers.
> Other than that there is some removal of code that was actually supposed
> to go into a later patch because it has dependencies that haven't been
> merged yet and some moving around of #defines and the device tree
> bindings documentation. Finally the driver now uses the DRM core's
> drm_compat_ioctl() instead of a custom and unimplemented (!) version.
(on the Harmony board's HDMI output; I'll test other boards/outputs later).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> This commit adds a KMS driver for the Tegra20 SoC. This includes basic
> support for host1x and the two display controllers found on the Tegra20
> SoC. Each display controller can drive a separate RGB/LVDS output.
> Signed-off-by: Thierry Reding <thierry.red...@avionic-design.de>
> ---
> Changes in v2:
> - drop Linux-specific drm subdirectory for DT bindings documentation
> - remove display helper leftovers that belong in a later patch
> - reuse debugfs infrastructure provided by the DRM core
> - move vblank syncpoint defines to dc.h
> - use drm_compat_ioctl()
> This commit adds support for the HDMI output on the Tegra20 SoC. Only
> one such output is available, but it can be driven by either of the two
> display controllers.
> A lot of work on this patch has been contributed by NVIDIA's Mark Zhang
> <ma...@nvidia.com> and many other people at NVIDIA were very helpful in
> getting the HDMI support and surrounding infrastructure to work.
> Signed-off-by: Thierry Reding <thierry.red...@avionic-design.de>
> ---
> Changes in v2:
> - reuse debugfs infrastructure provided by the DRM core
> This commit adds a KMS driver for the Tegra20 SoC. This includes basic
> support for host1x and the two display controllers found on the Tegra20
> SoC. Each display controller can drive a separate RGB/LVDS output.
Thanks Thierry for the hard work. I noticed the same thing Stephen
noticed earlier - I couldn't compile the driver.
Other than that, there were a couple of differences to the tree we've
worked with and tested earlier: Tegra30 support was dropped and as a
consequence so was IOMMU support.
I'm perfectly fine with leaving out IOMMU support, because we had some
problems with that and dma-buf. We were getting a bit worried about how
to implement mapping and re-mapping dma-buf's without causing duplicate
IOMMU mappings.
> That should be fixed with v2 I posted a few hours ago.
That's true.
> Yes, Tegra30 support will follow in a separate patch. As for IOMMU
> support it should eventually be made completely transparent, but I'm not
> opposed to adding a patch with explicit IOMMU support back in if it
> turns out that it can't be done transparently.
The trouble with this is that the generic case demands that we support
each device having a separate address space. But, on Tegra30 SMMU has
only four address spaces, so in Tegra30 case we actually end up using
the same address space for multiple devices.
Second problem is trying to avoid double mapping. With 2D acceleration,
we need support for allocating buffers, and mapping them to DC, host1x
and 2D. DMA Mapping API has a problem in that it doesn't allow just
allocating - it forces mapping the buffer to a device at the same time.
So, when 2D submit contains a reference to a dma-buf, the buffer has
already been mapped to some device, but we don't have a good way of
detecting that. We end up mapping it again, which is a performance
killer and possibly a coherence problem.
Fortunately this is all related to 2D acceleration, so we can defer both
of these problems for now.
> On Tue, Nov 13, 2012 at 03:15:47PM +0800, Mark Zhang wrote:
>> On 11/13/2012 05:55 AM, Thierry Reding wrote:
>>> This commit adds a KMS driver for the Tegra20 SoC. This includes basic
>>> support for host1x and the two display controllers found on the Tegra20
>>> SoC. Each display controller can drive a separate RGB/LVDS output.
>>> Signed-off-by: Thierry Reding <thierry.red...@avionic-design.de>
>>> ---
>>> Changes in v2:
>>> - drop Linux-specific drm subdirectory for DT bindings documentation
>>> - remove display helper leftovers that belong in a later patch
>>> - reuse debugfs infrastructure provided by the DRM core
>>> - move vblank syncpoint defines to dc.h
>>> - use drm_compat_ioctl()
>> Just for curious, according to my testing, why the "CONFIG_CMA" is not
>> enabled while DRM_GEM_CMA_HELPER & DRM_KMS_CMA_HELPER are enabled here?
> The reason is that CMA doesn't actually provide any API for drivers to
> use and in fact unless you use very large buffers you could indeed run
> this code on top of a non-CMA kernel and it will likely even work.
Okay. But I think it's better to turn on CMA defaultly. During my
testing, it's hard to allocate more 2MB without CMA...
>> Why we need this "drm_active" list? We can combine this function and
>> function "host1x_remove_drm_client" and free the drm client just here.
>> It's useless after host1x clients registered themselves.
> The list is used to properly remove all clients and resources when the
> module is unloaded. Granted, this code isn't executed if you don't build
> the driver as a loadable module, but it should still be a supported use-
> case.
My opinion is, after registration is completed, host1x_drm_client is
useless, host1x_client is enough for follow-up operations.
I still don't get how this is related with building the driver into the
kernel or as a kernel module, so if something I misunderstood, please
let me know it. Thanks.
>> Although this code works but I think it looks confusing.
>> "host1x_drm_exit" calls every client's "drm_exit" callback then free all
>> the host1x clients, but this function is placed inside a loop.
>> I think the better way is, free this host1x_client first, then remove it
>> from host1x's "clients" list, finally free the host1x(call
>> host1x_drm_exit) when the "clients" list get empty.
> But that would be the same thing, only slightly more explicit. I find
> that the above reads quite well as: look through the list of active
> clients and if the client to be removed is in that list, teardown the
> DRM part.
All right, this is just coding style problem and I think your words make
sense as well.
>> The i2c adapter may not be ready at this time. For Tegra 2, the I2C bus
>> for HDMI is not dedicated and we need the i2cmux driver loaded before
>> this i2c can be used. It proved that sometimes i2cmux driver loads after
>> drm driver.
>> So we need to add some logics to support driver probe deferral here.
>> Anyway, I'm just want you know about this and we can improve this later.
> Good point. Unfortunately tegra_output_init() isn't always used from
> within .probe(), so it isn't quite easy to handle deferred probe here.
> I'll have to take a look at how to solve this properly.
>> Okay, seems this works with the "CLK_DUPLICATE" in tegra20_clocks_data.c.
>> I think the purpose of all these is to make sure the dc has a correct
>> parent clock. Hm... But I feel this may bring confusing to do dc clock
>> settings in a drm output component.
> How do you think this would be confusing?
I just feel that all dc related works should be handled in crtc while
not in output. Anyway, this is not a big deal and I think the current
implementation also makes sense.
> This commit adds a KMS driver for the Tegra20 SoC. This includes basic
> support for host1x and the two display controllers found on the Tegra20
> SoC. Each display controller can drive a separate RGB/LVDS output.
I have tested this with further patches to enable display on my ventana
board. My only problem is that we don't force CMA on, but we can add
that later.
> On Tue, Nov 13, 2012 at 04:49:24PM +0800, Mark Zhang wrote:
>> On 11/13/2012 03:48 PM, Thierry Reding wrote:
>>>> Old Signed by an unknown key
>>> On Tue, Nov 13, 2012 at 03:15:47PM +0800, Mark Zhang wrote:
>>>> On 11/13/2012 05:55 AM, Thierry Reding wrote:
>>>>> This commit adds a KMS driver for the Tegra20 SoC. This includes basic
>>>>> support for host1x and the two display controllers found on the Tegra20
>>>>> SoC. Each display controller can drive a separate RGB/LVDS output.
>>>>> Signed-off-by: Thierry Reding <thierry.red...@avionic-design.de>
>>>>> ---
>>>>> Changes in v2:
>>>>> - drop Linux-specific drm subdirectory for DT bindings documentation
>>>>> - remove display helper leftovers that belong in a later patch
>>>>> - reuse debugfs infrastructure provided by the DRM core
>>>>> - move vblank syncpoint defines to dc.h
>>>>> - use drm_compat_ioctl()
>>>> Just for curious, according to my testing, why the "CONFIG_CMA" is not
>>>> enabled while DRM_GEM_CMA_HELPER & DRM_KMS_CMA_HELPER are enabled here?
>>> The reason is that CMA doesn't actually provide any API for drivers to
>>> use and in fact unless you use very large buffers you could indeed run
>>> this code on top of a non-CMA kernel and it will likely even work.
>> Okay. But I think it's better to turn on CMA defaultly. During my
>> testing, it's hard to allocate more 2MB without CMA...
> CMA is enabled by default in one of the Tegra default configuration
> patches in my tegra/next branch. I will submit that patch to Stephen
> when the 3.8 cycle starts, so that it'll be automatically enabled along
> with the DRM driver.
> But I don't think it makes sense to couple it to the DRM_TEGRA symbol as
> it isn't strictly required.
Yes. We don't need to touch CMA in our Kconfig. In my opinion, right now
we're relying on the DRM_GEM_CMA_HELPER which should turn on CMA when
it's been selected.
>>>> Why we need this "drm_active" list? We can combine this function and
>>>> function "host1x_remove_drm_client" and free the drm client just here.
>>>> It's useless after host1x clients registered themselves.
>>> The list is used to properly remove all clients and resources when the
>>> module is unloaded. Granted, this code isn't executed if you don't build
>>> the driver as a loadable module, but it should still be a supported use-
>>> case.
>> My opinion is, after registration is completed, host1x_drm_client is
>> useless, host1x_client is enough for follow-up operations.
>> I still don't get how this is related with building the driver into the
>> kernel or as a kernel module, so if something I misunderstood, please
>> let me know it. Thanks.
> I can take another look at this and see if it can be further simplified.
> This was actually a rather tricky part to get right, so I'm naturally a
> bit hesitant to touch it.
Okay. I recall I did some changes on this part about 3 month ago in a
patch named "drm: Add T30 support - host1x". So maybe you can know what
I mean by reading that patch.
> On Tue, Nov 13, 2012 at 05:49:28PM +0800, Mark Zhang wrote:
>> On 11/13/2012 05:37 PM, Thierry Reding wrote:
>>>> Old Signed by an unknown key
>>> On Tue, Nov 13, 2012 at 04:49:24PM +0800, Mark Zhang wrote:
>>>> On 11/13/2012 03:48 PM, Thierry Reding wrote:
>>>>>> Old Signed by an unknown key
>>>>> On Tue, Nov 13, 2012 at 03:15:47PM +0800, Mark Zhang wrote:
>>>>>> On 11/13/2012 05:55 AM, Thierry Reding wrote:
>>>>>>> This commit adds a KMS driver for the Tegra20 SoC. This includes basic
>>>>>>> support for host1x and the two display controllers found on the Tegra20
>>>>>>> SoC. Each display controller can drive a separate RGB/LVDS output.
>>>>>>> Signed-off-by: Thierry Reding <thierry.red...@avionic-design.de>
>>>>>>> ---
>>>>>>> Changes in v2:
>>>>>>> - drop Linux-specific drm subdirectory for DT bindings documentation
>>>>>>> - remove display helper leftovers that belong in a later patch
>>>>>>> - reuse debugfs infrastructure provided by the DRM core
>>>>>>> - move vblank syncpoint defines to dc.h
>>>>>>> - use drm_compat_ioctl()
>>>>>> Just for curious, according to my testing, why the "CONFIG_CMA" is not
>>>>>> enabled while DRM_GEM_CMA_HELPER & DRM_KMS_CMA_HELPER are enabled here?
>>>>> The reason is that CMA doesn't actually provide any API for drivers to
>>>>> use and in fact unless you use very large buffers you could indeed run
>>>>> this code on top of a non-CMA kernel and it will likely even work.
>>>> Okay. But I think it's better to turn on CMA defaultly. During my
>>>> testing, it's hard to allocate more 2MB without CMA...
>>> CMA is enabled by default in one of the Tegra default configuration
>>> patches in my tegra/next branch. I will submit that patch to Stephen
>>> when the 3.8 cycle starts, so that it'll be automatically enabled along
>>> with the DRM driver.
>>> But I don't think it makes sense to couple it to the DRM_TEGRA symbol as
>>> it isn't strictly required.
>> Yes. We don't need to touch CMA in our Kconfig. In my opinion, right now
>> we're relying on the DRM_GEM_CMA_HELPER which should turn on CMA when
>> it's been selected.
> Again, I don't think CMA should be selected by those either as the
> helpers will work fine if CMA is disabled (their name is a bit
> unfortunate). It's just that they won't be able to allocate very large
> buffers.
> So I think the correct way is to select CMA in the Tegra default
> configuration to make it explicit that Tegra wants to use the CMA for
> large contiguous buffer allocations.
>>>>>> Why we need this "drm_active" list? We can combine this function and
>>>>>> function "host1x_remove_drm_client" and free the drm client just here.
>>>>>> It's useless after host1x clients registered themselves.
>>>>> The list is used to properly remove all clients and resources when the
>>>>> module is unloaded. Granted, this code isn't executed if you don't build
>>>>> the driver as a loadable module, but it should still be a supported use-
>>>>> case.
>>>> My opinion is, after registration is completed, host1x_drm_client is
>>>> useless, host1x_client is enough for follow-up operations.
>>>> I still don't get how this is related with building the driver into the
>>>> kernel or as a kernel module, so if something I misunderstood, please
>>>> let me know it. Thanks.
>>> I can take another look at this and see if it can be further simplified.
>>> This was actually a rather tricky part to get right, so I'm naturally a
>>> bit hesitant to touch it.
>> Okay. I recall I did some changes on this part about 3 month ago in a
>> patch named "drm: Add T30 support - host1x". So maybe you can know what
>> I mean by reading that patch.
> Yes, I remember the patch. Unfortunately the result of applying that
> patch was that unloading the module no longer worked properly.
Okay. I'll take a look at this part as well when I'm free.
