| Message ID | 20220218113952.3077606-9-mperttunen@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Host1x context isolation support | expand |
18.02.2022 14:39, Mikko Perttunen пишет: > +static int vic_get_streamid_offset(struct tegra_drm_client *client) > +{ > + struct vic *vic = to_vic(client); > + int err; > + > + err = vic_load_firmware(vic); You can't invoke vic_load_firmware() while RPM is suspended. Either replace this with RPM get/put or do something else. > + if (err < 0) > + return err; > + > + if (vic->can_use_context) > + return 0x30; > + else > + return -ENOTSUPP; If (!vic->can_use_context) return -ENOTSUPP; return 0x30;
19.02.2022 21:49, Dmitry Osipenko пишет: > 18.02.2022 14:39, Mikko Perttunen пишет: >> +static int vic_get_streamid_offset(struct tegra_drm_client *client) >> +{ >> + struct vic *vic = to_vic(client); >> + int err; >> + >> + err = vic_load_firmware(vic); > > You can't invoke vic_load_firmware() while RPM is suspended. Either > replace this with RPM get/put or do something else. > >> + if (err < 0) >> + return err; >> + >> + if (vic->can_use_context) >> + return 0x30; >> + else >> + return -ENOTSUPP; > > If (!vic->can_use_context) > return -ENOTSUPP; > > return 0x30; and s/ENOTSUPP/EOPNOTSUPP/
On 2/19/22 20:54, Dmitry Osipenko wrote: > 19.02.2022 21:49, Dmitry Osipenko пишет: >> 18.02.2022 14:39, Mikko Perttunen пишет: >>> +static int vic_get_streamid_offset(struct tegra_drm_client *client) >>> +{ >>> + struct vic *vic = to_vic(client); >>> + int err; >>> + >>> + err = vic_load_firmware(vic); >> >> You can't invoke vic_load_firmware() while RPM is suspended. Either >> replace this with RPM get/put or do something else. Why not, I'm not seeing any HW accesses in vic_load_firmware? Although it looks like it might race with the vic_load_firmware call in vic_runtime_resume which probably needs to be fixed. >> >>> + if (err < 0) >>> + return err; >>> + >>> + if (vic->can_use_context) >>> + return 0x30; >>> + else >>> + return -ENOTSUPP; >> >> If (!vic->can_use_context) >> return -ENOTSUPP; >> >> return 0x30; > > and s/ENOTSUPP/EOPNOTSUPP/ Ok. Mikko
On 2022-02-18 11:39, Mikko Perttunen via iommu wrote: > Implement the get_streamid_offset required for supporting context > isolation. Since old firmware cannot support context isolation > without hacks that we don't want to implement, check the firmware > binary to see if context isolation should be enabled. > > Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> > --- > drivers/gpu/drm/tegra/vic.c | 38 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c > index 1e342fa3d27b..2863ee5e0e67 100644 > --- a/drivers/gpu/drm/tegra/vic.c > +++ b/drivers/gpu/drm/tegra/vic.c > @@ -38,6 +38,8 @@ struct vic { > struct clk *clk; > struct reset_control *rst; > > + bool can_use_context; > + > /* Platform configuration */ > const struct vic_config *config; > }; > @@ -229,6 +231,7 @@ static int vic_load_firmware(struct vic *vic) > { > struct host1x_client *client = &vic->client.base; > struct tegra_drm *tegra = vic->client.drm; > + u32 fce_bin_data_offset; > dma_addr_t iova; > size_t size; > void *virt; > @@ -277,6 +280,25 @@ static int vic_load_firmware(struct vic *vic) > vic->falcon.firmware.phys = phys; > } > > + /* > + * Check if firmware is new enough to not require mapping firmware > + * to data buffer domains. > + */ > + fce_bin_data_offset = *(u32 *)(virt + VIC_UCODE_FCE_DATA_OFFSET); > + > + if (!vic->config->supports_sid) { > + vic->can_use_context = false; > + } else if (fce_bin_data_offset != 0x0 && fce_bin_data_offset != 0xa5a5a5a5) { > + /* > + * Firmware will access FCE through STREAMID0, so context > + * isolation cannot be used. > + */ > + vic->can_use_context = false; > + dev_warn_once(vic->dev, "context isolation disabled due to old firmware\n"); > + } else { > + vic->can_use_context = true; > + } > + > return 0; > > cleanup: > @@ -358,10 +380,26 @@ static void vic_close_channel(struct tegra_drm_context *context) > host1x_channel_put(context->channel); > } > > +static int vic_get_streamid_offset(struct tegra_drm_client *client) > +{ > + struct vic *vic = to_vic(client); > + int err; > + > + err = vic_load_firmware(vic); > + if (err < 0) > + return err; > + > + if (vic->can_use_context) > + return 0x30; > + else > + return -ENOTSUPP; > +} > + > static const struct tegra_drm_client_ops vic_ops = { > .open_channel = vic_open_channel, > .close_channel = vic_close_channel, > .submit = tegra_drm_submit, > + .get_streamid_offset = vic_get_streamid_offset, The patch order seems off here, since the .get_streamid_offset member isn't defined yet. Robin. > }; > > #define NVIDIA_TEGRA_124_VIC_FIRMWARE "nvidia/tegra124/vic03_ucode.bin"
On 2/21/22 19:27, Robin Murphy wrote: > On 2022-02-18 11:39, Mikko Perttunen via iommu wrote: >> Implement the get_streamid_offset required for supporting context >> isolation. Since old firmware cannot support context isolation >> without hacks that we don't want to implement, check the firmware >> binary to see if context isolation should be enabled. >> >> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> >> --- >> drivers/gpu/drm/tegra/vic.c | 38 +++++++++++++++++++++++++++++++++++++ >> 1 file changed, 38 insertions(+) >> >> diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c >> index 1e342fa3d27b..2863ee5e0e67 100644 >> --- a/drivers/gpu/drm/tegra/vic.c >> +++ b/drivers/gpu/drm/tegra/vic.c >> @@ -38,6 +38,8 @@ struct vic { >> struct clk *clk; >> struct reset_control *rst; >> + bool can_use_context; >> + >> /* Platform configuration */ >> const struct vic_config *config; >> }; >> @@ -229,6 +231,7 @@ static int vic_load_firmware(struct vic *vic) >> { >> struct host1x_client *client = &vic->client.base; >> struct tegra_drm *tegra = vic->client.drm; >> + u32 fce_bin_data_offset; >> dma_addr_t iova; >> size_t size; >> void *virt; >> @@ -277,6 +280,25 @@ static int vic_load_firmware(struct vic *vic) >> vic->falcon.firmware.phys = phys; >> } >> + /* >> + * Check if firmware is new enough to not require mapping firmware >> + * to data buffer domains. >> + */ >> + fce_bin_data_offset = *(u32 *)(virt + VIC_UCODE_FCE_DATA_OFFSET); >> + >> + if (!vic->config->supports_sid) { >> + vic->can_use_context = false; >> + } else if (fce_bin_data_offset != 0x0 && fce_bin_data_offset != >> 0xa5a5a5a5) { >> + /* >> + * Firmware will access FCE through STREAMID0, so context >> + * isolation cannot be used. >> + */ >> + vic->can_use_context = false; >> + dev_warn_once(vic->dev, "context isolation disabled due to >> old firmware\n"); >> + } else { >> + vic->can_use_context = true; >> + } >> + >> return 0; >> cleanup: >> @@ -358,10 +380,26 @@ static void vic_close_channel(struct >> tegra_drm_context *context) >> host1x_channel_put(context->channel); >> } >> +static int vic_get_streamid_offset(struct tegra_drm_client *client) >> +{ >> + struct vic *vic = to_vic(client); >> + int err; >> + >> + err = vic_load_firmware(vic); >> + if (err < 0) >> + return err; >> + >> + if (vic->can_use_context) >> + return 0x30; >> + else >> + return -ENOTSUPP; >> +} >> + >> static const struct tegra_drm_client_ops vic_ops = { >> .open_channel = vic_open_channel, >> .close_channel = vic_close_channel, >> .submit = tegra_drm_submit, >> + .get_streamid_offset = vic_get_streamid_offset, > > The patch order seems off here, since the .get_streamid_offset member > isn't defined yet. > > Robin. Indeed, will fix. Thanks, Mikko > >> }; >> #define NVIDIA_TEGRA_124_VIC_FIRMWARE "nvidia/tegra124/vic03_ucode.bin"
21.02.2022 14:44, Mikko Perttunen пишет: > On 2/19/22 20:54, Dmitry Osipenko wrote: >> 19.02.2022 21:49, Dmitry Osipenko пишет: >>> 18.02.2022 14:39, Mikko Perttunen пишет: >>>> +static int vic_get_streamid_offset(struct tegra_drm_client *client) >>>> +{ >>>> + struct vic *vic = to_vic(client); >>>> + int err; >>>> + >>>> + err = vic_load_firmware(vic); >>> >>> You can't invoke vic_load_firmware() while RPM is suspended. Either >>> replace this with RPM get/put or do something else. > > Why not, I'm not seeing any HW accesses in vic_load_firmware? Although > it looks like it might race with the vic_load_firmware call in > vic_runtime_resume which probably needs to be fixed. It was not clear from the function's name that h/w is untouched, I read "load" as "upload" and then looked at vic_runtime_resume(). I'd rename vic_load_firmware() to vic_prepare_firmware_image(). And yes, technically lock is needed.
On 2/21/22 22:10, Dmitry Osipenko wrote: > 21.02.2022 14:44, Mikko Perttunen пишет: >> On 2/19/22 20:54, Dmitry Osipenko wrote: >>> 19.02.2022 21:49, Dmitry Osipenko пишет: >>>> 18.02.2022 14:39, Mikko Perttunen пишет: >>>>> +static int vic_get_streamid_offset(struct tegra_drm_client *client) >>>>> +{ >>>>> + struct vic *vic = to_vic(client); >>>>> + int err; >>>>> + >>>>> + err = vic_load_firmware(vic); >>>> >>>> You can't invoke vic_load_firmware() while RPM is suspended. Either >>>> replace this with RPM get/put or do something else. >> >> Why not, I'm not seeing any HW accesses in vic_load_firmware? Although >> it looks like it might race with the vic_load_firmware call in >> vic_runtime_resume which probably needs to be fixed. > > It was not clear from the function's name that h/w is untouched, I read > "load" as "upload" and then looked at vic_runtime_resume(). I'd rename > vic_load_firmware() to vic_prepare_firmware_image(). > > And yes, technically lock is needed. Yep, I'll consider renaming it. Mikko
22.02.2022 11:27, Mikko Perttunen пишет: > On 2/21/22 22:10, Dmitry Osipenko wrote: >> 21.02.2022 14:44, Mikko Perttunen пишет: >>> On 2/19/22 20:54, Dmitry Osipenko wrote: >>>> 19.02.2022 21:49, Dmitry Osipenko пишет: >>>>> 18.02.2022 14:39, Mikko Perttunen пишет: >>>>>> +static int vic_get_streamid_offset(struct tegra_drm_client *client) >>>>>> +{ >>>>>> + struct vic *vic = to_vic(client); >>>>>> + int err; >>>>>> + >>>>>> + err = vic_load_firmware(vic); >>>>> >>>>> You can't invoke vic_load_firmware() while RPM is suspended. Either >>>>> replace this with RPM get/put or do something else. >>> >>> Why not, I'm not seeing any HW accesses in vic_load_firmware? Although >>> it looks like it might race with the vic_load_firmware call in >>> vic_runtime_resume which probably needs to be fixed. >> >> It was not clear from the function's name that h/w is untouched, I read >> "load" as "upload" and then looked at vic_runtime_resume(). I'd rename >> vic_load_firmware() to vic_prepare_firmware_image(). >> >> And yes, technically lock is needed. > > Yep, I'll consider renaming it. Looking at this all again, I'd suggest to change: int get_streamid_offset(client) to: int get_streamid_offset(client, *offset) and bail out if get_streamid_offset() returns error. It's never okay to ignore errors.
On 2/22/22 12:46, Dmitry Osipenko wrote: > 22.02.2022 11:27, Mikko Perttunen пишет: >> On 2/21/22 22:10, Dmitry Osipenko wrote: >>> 21.02.2022 14:44, Mikko Perttunen пишет: >>>> On 2/19/22 20:54, Dmitry Osipenko wrote: >>>>> 19.02.2022 21:49, Dmitry Osipenko пишет: >>>>>> 18.02.2022 14:39, Mikko Perttunen пишет: >>>>>>> +static int vic_get_streamid_offset(struct tegra_drm_client *client) >>>>>>> +{ >>>>>>> + struct vic *vic = to_vic(client); >>>>>>> + int err; >>>>>>> + >>>>>>> + err = vic_load_firmware(vic); >>>>>> >>>>>> You can't invoke vic_load_firmware() while RPM is suspended. Either >>>>>> replace this with RPM get/put or do something else. >>>> >>>> Why not, I'm not seeing any HW accesses in vic_load_firmware? Although >>>> it looks like it might race with the vic_load_firmware call in >>>> vic_runtime_resume which probably needs to be fixed. >>> >>> It was not clear from the function's name that h/w is untouched, I read >>> "load" as "upload" and then looked at vic_runtime_resume(). I'd rename >>> vic_load_firmware() to vic_prepare_firmware_image(). >>> >>> And yes, technically lock is needed. >> >> Yep, I'll consider renaming it. > > Looking at this all again, I'd suggest to change: > > int get_streamid_offset(client) > > to: > > int get_streamid_offset(client, *offset) > > and bail out if get_streamid_offset() returns error. It's never okay to > ignore errors. Sure, seems reasonable. We'll still need some error code to indicate that context isolation isn't available for the engine and continue on in that case but that's better than just ignoring all of them. Mikko
22.02.2022 13:54, Mikko Perttunen пишет: > On 2/22/22 12:46, Dmitry Osipenko wrote: >> 22.02.2022 11:27, Mikko Perttunen пишет: >>> On 2/21/22 22:10, Dmitry Osipenko wrote: >>>> 21.02.2022 14:44, Mikko Perttunen пишет: >>>>> On 2/19/22 20:54, Dmitry Osipenko wrote: >>>>>> 19.02.2022 21:49, Dmitry Osipenko пишет: >>>>>>> 18.02.2022 14:39, Mikko Perttunen пишет: >>>>>>>> +static int vic_get_streamid_offset(struct tegra_drm_client >>>>>>>> *client) >>>>>>>> +{ >>>>>>>> + struct vic *vic = to_vic(client); >>>>>>>> + int err; >>>>>>>> + >>>>>>>> + err = vic_load_firmware(vic); >>>>>>> >>>>>>> You can't invoke vic_load_firmware() while RPM is suspended. Either >>>>>>> replace this with RPM get/put or do something else. >>>>> >>>>> Why not, I'm not seeing any HW accesses in vic_load_firmware? Although >>>>> it looks like it might race with the vic_load_firmware call in >>>>> vic_runtime_resume which probably needs to be fixed. >>>> >>>> It was not clear from the function's name that h/w is untouched, I read >>>> "load" as "upload" and then looked at vic_runtime_resume(). I'd rename >>>> vic_load_firmware() to vic_prepare_firmware_image(). >>>> >>>> And yes, technically lock is needed. >>> >>> Yep, I'll consider renaming it. >> >> Looking at this all again, I'd suggest to change: >> >> int get_streamid_offset(client) >> >> to: >> >> int get_streamid_offset(client, *offset) >> >> and bail out if get_streamid_offset() returns error. It's never okay to >> ignore errors. > > Sure, seems reasonable. We'll still need some error code to indicate > that context isolation isn't available for the engine and continue on in > that case but that's better than just ignoring all of them. Yes, check for -EOPNOTSUPP and skip it.
diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c index 1e342fa3d27b..2863ee5e0e67 100644 --- a/drivers/gpu/drm/tegra/vic.c +++ b/drivers/gpu/drm/tegra/vic.c @@ -38,6 +38,8 @@ struct vic { struct clk *clk; struct reset_control *rst; + bool can_use_context; + /* Platform configuration */ const struct vic_config *config; }; @@ -229,6 +231,7 @@ static int vic_load_firmware(struct vic *vic) { struct host1x_client *client = &vic->client.base; struct tegra_drm *tegra = vic->client.drm; + u32 fce_bin_data_offset; dma_addr_t iova; size_t size; void *virt; @@ -277,6 +280,25 @@ static int vic_load_firmware(struct vic *vic) vic->falcon.firmware.phys = phys; } + /* + * Check if firmware is new enough to not require mapping firmware + * to data buffer domains. + */ + fce_bin_data_offset = *(u32 *)(virt + VIC_UCODE_FCE_DATA_OFFSET); + + if (!vic->config->supports_sid) { + vic->can_use_context = false; + } else if (fce_bin_data_offset != 0x0 && fce_bin_data_offset != 0xa5a5a5a5) { + /* + * Firmware will access FCE through STREAMID0, so context + * isolation cannot be used. + */ + vic->can_use_context = false; + dev_warn_once(vic->dev, "context isolation disabled due to old firmware\n"); + } else { + vic->can_use_context = true; + } + return 0; cleanup: @@ -358,10 +380,26 @@ static void vic_close_channel(struct tegra_drm_context *context) host1x_channel_put(context->channel); } +static int vic_get_streamid_offset(struct tegra_drm_client *client) +{ + struct vic *vic = to_vic(client); + int err; + + err = vic_load_firmware(vic); + if (err < 0) + return err; + + if (vic->can_use_context) + return 0x30; + else + return -ENOTSUPP; +} + static const struct tegra_drm_client_ops vic_ops = { .open_channel = vic_open_channel, .close_channel = vic_close_channel, .submit = tegra_drm_submit, + .get_streamid_offset = vic_get_streamid_offset, }; #define NVIDIA_TEGRA_124_VIC_FIRMWARE "nvidia/tegra124/vic03_ucode.bin"
Implement the get_streamid_offset required for supporting context isolation. Since old firmware cannot support context isolation without hacks that we don't want to implement, check the firmware binary to see if context isolation should be enabled. Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> --- drivers/gpu/drm/tegra/vic.c | 38 +++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+)