Re: [PATCH 3/4] drm/tegra: Add VIC support
From: Arto Merilainen
Date: Mon May 25 2015 - 04:25:48 EST
Hi Thierry,
Thank you for your thorough analysis - and sorry for a bunch of very
silly mistakes.
I am skipping most trivial parts and focus on the trickier comments and
questions.
On 05/22/2015 02:47 PM, Thierry Reding wrote:
+
+ struct tegra_bo *ucode_bo;
+ bool ucode_valid;
+ void *ucode_vaddr;
+
+ bool booted;
There are a bunch of other drivers that use a Falcon and they will all
need to use similar data to this to deal with the firmware and all. I
would like to see that code to be made into a Falcon library so that it
can be reused in a meaningful way.
Roughly this would look like this:
struct falcon {
struct device *dev;
...
};
int falcon_init(struct falcon *falcon, struct device *dev,
void __iomem *regs);
int falcon_load_firmware(struct falcon *falcon, const char *filename);
int falcon_exit(struct falcon *falcon);
int falcon_boot(struct falcon *falcon);
There are two issues in above scheme..:
- Memory allocation. Despite I have explicitly mentioned that the series
has been tested only with iommu disabled, I would prefer trying to keep
an option to use it later. Depending how well we want to isolate the
falcon library from other parts of tegradrm, the library may not have
ability to map anything to the tegradrm iommu domain.
- The firmware images may not hold only Falcon firmware. Already in VIC
case we have two firmwares: One for Falcon, another for FCE.
To overcome the above issues, I would prefer dropping
falcon_load_firmware() and keeping firmware image specifics inside the
client driver and giving data related to Falcon as a parameter for
falcon_boot(). Would this be ok?
+
+ /* for firewall - this determines if method 1 should be regarded
+ * as an address register */
+ bool method_data_is_addr_reg;
+};
I think it'd be best to incorporate that functionality into the firewall
so that we can deal with it more centrally rather that duplicate this in
all drivers.
Do you have a concrete suggestion how this should be done? Firewall has
no access to the driver specifics and in VIC case the VIC methods
themselves define whether the METHOD1 includes address or not.
+static int vic_dma_pa_to_internal_256b(struct vic *vic, phys_addr_t pa,
+ u32 internal_offset, bool imem)
The name is confusing. Without looking at the code I'd expect this to
perform some kind of conversion from a physical address to some internal
address, but if I understand correctly this actually copies code into
the Falcon instruction or data memories. A better name I think would be:
static int falcon_load_chunk(struct falcon *falcon, phys_addr_t phys,
unsigned long offset, enum falcon_memory target);
Note that this is now part of the Falcon library because I've seen
identical code used in several other drivers. Also the bool argument is
now an enumeration, which makes it much easier to read. Compare:
err = vic_dma_pa_to_internal_256b(vic, phys, offset, true);
and
err = falcon_load_chunk(&vic->falcon, phys, offset, FALCON_MEMORY_IMEM);
Sounds ok.
Is there a specific term in Falcon-speak for these 256 byte blocks?
"chunk" is a little awkward.
Unfortunately I am not aware that there would be - maybe "block"?
+ if (ucode.bin_header->bin_ver != 1) {
+ dev_err(vic->dev, "unsupported firmware version");
+ return -ENOENT;
Why not -EINVAL here, too?
We can interpret the issue two ways..: The given firmware is invalid
(-EINVAL) or that there was no supported firmware entry available (-ENOENT).
I do not have strong opinions and will change this to -EINVAL.
+ vic->ucode_bo = tegra_bo_create(dev, ucode_fw->size, 0);
+ if (IS_ERR(vic->ucode_bo)) {
+ dev_err(vic->dev, "dma memory allocation failed");
+ err = PTR_ERR(vic->ucode_bo);
+ goto err_alloc_iova;
+ }
Erm... no. Please don't use tegra_bo_create() to allocate these buffers.
tegra_bo_create() creates GEM objects and firmware doesn't qualify as a
GEM object.
Can't you use the DMA API here?
The firmware must be mapped to the IOMMU domain into which VIC is
attached - and I would prefer keeping the door open for enabling iommu
on VIC. This was the simplest way to get a buffer that is allocated to
the tegradrm iommu domain.
Should I add a function for allocating memory without making a gem
object or should I keep memory allocation here and simply add functions
for mapping it into tegradrm domain?
+static int vic_boot(struct device *dev)
+{
+ struct vic *vic = dev_get_drvdata(dev);
+ u32 offset;
+ int err = 0;
+
+ if (vic->booted)
+ return 0;
+
+ if (!vic->ucode_valid) {
+ err = vic_read_ucode(vic);
+ if (err)
+ return err;
+ }
I think this is the wrong place to do this. It's good in that it's as
late as possible, but vic_boot() is kind of the wrong place. I think you
should load the firmware and upload it into the Falcon within the
->open_channel() implementation, prior to the vic_boot() call. There is
also no need to reload the firmware every time a VIC channel is opened.
I think loading the firmware could be done in ->init() instead.
I'd prefer pushing this simply inside ->open_channel().
Please correct me if I am wrong here, but ->init() is usually called
before rootfs has been mounted and the firmware may not be available at
that point => initialization fails
+ dev_err(dev, "boot failed due to timeout");
+ return err;
+ }
+
+ /* Set application id and set-up FCE ucode address */
+ vic_writel(vic, NVA0B6_VIDEO_COMPOSITOR_SET_APPLICATION_ID >> 2,
+ NV_PVIC_FALCON_METHOD_0);
+ vic_writel(vic, 1, NV_PVIC_FALCON_METHOD_1);
+ vic_writel(vic, NVA0B6_VIDEO_COMPOSITOR_SET_FCE_UCODE_SIZE >> 2,
+ NV_PVIC_FALCON_METHOD_0);
+ vic_writel(vic, vic->fce.size, NV_PVIC_FALCON_METHOD_1);
Looks like this is already a case of device-specific case that's part of
the boot sequence, so it seems like we're going to need these low-level
helpers for VIC already.
Correct; Everything related to FCE is VIC specific.
+static int vic_exit(struct host1x_client *client)
+{
+ struct tegra_drm_client *drm = host1x_to_drm_client(client);
+ struct drm_device *dev = dev_get_drvdata(client->parent);
+ struct tegra_drm *tegra = dev->dev_private;
+ struct vic *vic = to_vic(drm);
+ int err;
+
+ err = tegra_drm_unregister_client(tegra, drm);
+ if (err < 0)
+ return err;
+
+ host1x_syncpt_free(client->syncpts[0]);
+ host1x_channel_free(vic->channel);
+
+ /* ucode is no longer available. release it */
This comment isn't accurate. The microcode won't be available after
you've released it. Perhaps you meant to say "no longer needed"?
Sounds better.
+ if (vic->ucode_valid) {
+ /* first, ensure that vic is not using it */
+ reset_control_assert(vic->rst);
+ udelay(10);
+ reset_control_deassert(vic->rst);
+
+ /* ..then release the ucode */
+ if (!vic->ucode_bo->vaddr)
+ vunmap(vic->ucode_vaddr);
+ drm_gem_object_release(&vic->ucode_bo->gem);
+ vic->ucode_valid = false;
+ }
This now also makes the code unbalanced. You allocate the microcode
during ->open_channel(), but free it in ->exit(). The allocation should
happen in ->init() instead if it's freed in ->exit().
Please refer to my previous comment for ->init() allocation.
We can rework ->close_channel() to reset the engine and release the
firmware, however, firmware reading and booting is not free so I would
prefer keeping this in ->exit().
+ err = tegra_powergate_sequence_power_up(vic->config->powergate_id,
+ vic->clk, vic->rst);
+ if (err) {
+ dev_err(dev, "cannot turn on the device\n");
+ return err;
+ }
+
+ err = host1x_client_register(&vic->client.base);
+ if (err < 0) {
+ dev_err(dev, "failed to register host1x client: %d\n", err);
+ clk_disable_unprepare(vic->clk);
+ tegra_powergate_power_off(vic->config->powergate_id);
tegra_powergate_sequence_power_up() also deasserts the reset, so you
probably want to assert that here again. Maybe to make it easier you
could abstract this away into a vic_enable()/vic_disable() pair of
functions? Or perhaps you could even use runtime PM for this? Don't
worry about runtime PM if that complicates things too much, though.
Sounds good. I planned to introduce PM runtime support a bit later but I
can check if I can fit to this patch already.
+struct ucode_bin_header_v1_vic {
+ u32 bin_magic; /* 0x10de */
+ u32 bin_ver; /* cya, versioning of bin format (1) */
+ u32 bin_size; /* entire image size including this header */
+ u32 os_bin_header_offset;
+ u32 os_bin_data_offset;
+ u32 os_bin_size;
+ u32 fce_bin_header_offset;
+ u32 fce_bin_data_offset;
+ u32 fce_bin_size;
+};
+
+struct ucode_os_code_header_v1_vic {
+ u32 offset;
+ u32 size;
+};
+
+struct ucode_os_header_v1_vic {
+ u32 os_code_offset;
+ u32 os_code_size;
+ u32 os_data_offset;
+ u32 os_data_size;
+ u32 num_apps;
+ struct ucode_os_code_header_v1_vic *app_code;
+ struct ucode_os_code_header_v1_vic *app_data;
+ u32 *os_ovl_offset;
+ u32 *of_ovl_size;
+};
+
+struct ucode_fce_header_v1_vic {
+ u32 fce_ucode_offset;
+ u32 fce_ucode_buffer_size;
+ u32 fce_ucode_size;
+};
+
+struct ucode_v1_vic {
+ struct ucode_bin_header_v1_vic *bin_header;
+ struct ucode_os_header_v1_vic *os_header;
+ struct ucode_fce_header_v1_vic *fce_header;
+};
I'll assume that these are data structures shared by all other drivers
for Falcon driven engines, so they should probably go into the Falcon
library header as well.
ucode_os_header_v1_vic is the only common structure above. Please see my
earlier comment for the falcon library suggestion.
+#define NV_PVIC_MISC_PRI_VIC_CG 0x000016d0
+#define CG_IDLE_CG_DLY_CNT(val) ((val & 0x3f) << 0)
+#define CG_IDLE_CG_EN (1 << 6)
+#define CG_WAKEUP_DLY_CNT(val) ((val & 0xf) << 16)
These aren't in the TRM either, but I vaguely remember this being
tracked in an internal bug. Have bugs been filed to track documentation
of the other registers as well?
Yes, there is a bug for tracking that this register - and three others
you mentioned - get documented in TRM.
- Arto
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/