Integer Scaling support implementation to improve the viewing quality of Pixel Art games

[slattann]

Background:
Blurry outputs during up-scaling the video buffer, is a generic problem of graphics industry. One of the major reason behind this blurriness is the interpolation of pixel values used by most of the up-scaling hardware.

Nearest-neighbor is a scaling filter type, which works by filling in the missing color values in the up-scaled image with that of the coordinate-mapped nearest source pixel value.

Nearest-neighbor can produce (almost) non-blurry scaling outputs when the scaling ratio is complete integer. For example:

input buffer resolution: 1280x720(HD)
output buffer resolution: 3840x2160(UHD/4K)
scaling ratio (h) = 3840/1280 = 3
scaling ratio (v) = 2160/720 = 3
In such scenarios, we should try to pick Nearest-neighbor as scaling method when possible.

Some references:

• https://tanalin.com/en/articles/integer-scaling/
• https://www.reddit.com/r/nvidia/comments/9e27wk/its_2018_nvidia_wheres_integer_scaling/
• http://www.zeus-software.com/forum/viewtopic.php?t=2065
• https://forums.geforce.com/default/topic/844905/geforce-drivers/-feature-request-nonblurry-upscaling-at-integer-ratios/20/
• https://forums.tomshardware.com/threads/does-anybody-have-a-good-solution-to-scale-1080p-onto-a-4k-monitor.3217086/

This patch series:
To address this feature kernel driver exposes new CRTC/Plane property called
"SCALING_FILTER" and gives an option to userspace to set it based on the use
cases.
Kernel RFC can be found here: https://patchwork.freedesktop.org/series/73883/#rev6

This patchset add support for utilizing CRTC/Plane scaling filter property based on
client's usecase.

Here are the patches details:
Patch 1/4 --> [Settings] adding Display Engine (DE) HW scaling filter option into setting menu
Patch 2/4 --> [RetroPlayer] videofilter and stretch setting control
Patch 3/4 --> [GameFrame] Setting Game frame to Window size, to avoid GL scaling when DE HW could do
Patch 4/4 --> [DRMAtomic] add scaling_filter property support at plane level

Awaiting your reviews and what do you think on having Integer Scaling in Kodi. Thanks
-Sameer Lattannavar

[lrusak]

Please use clang-format as there is a bunch of formatting errors. It would be easier to correct this first rather than have us comment on each problem. Thanks!

[garbear]

A design constraint is that ServiceBroker.h isn't allowed in this file. See how other singletons are handled.

[slattann]

Ok will take care of this.

[garbear]

This is a cool feature. Why default it to off?

[slattann]

I agree with you, let me know if you want me to make it default ON.

[garbear]

Big yes

[slattann]

Thank you @garbear , I have done the changes and updated the PR, kindly let me know if anything else needs to be done from my side to take it forward.

[lrusak]

please use CDRMUtils::SupportsProperty. You may also want to use CDRMUtils::SupportsPropertyAndValue

[slattann]

CDRMUtils::SupportsProperty & CDRMUtils::SupportsPropertyAndValue check particular property & value passed exists or not, but in this particular case we are traversing through Property and when we find the property (SCALING_FILTER) and then traverse through its supported types ("Nearest Neighbor", "Default" ..etc) then we set type ("Nearest Neighbor") value.
But wherever needs to check the property existence, will use CDRMUtils::SupportsProperty & CDRMUtils::SupportsPropertyAndValue.
Ex:
if (SupportsProperty(m_gui_plane, "SCALING_FILTER"))
{
const std::shared_ptr settings =
CServiceBroker::GetSettingsComponent()->GetSettings();
settings->GetSetting(SETTING_VIDEOSCREEN_HW_SCALING_FILTER)->SetVisible(true);
}

[lrusak]

I disagree, you could simply do

if (SupportsPropertyAndValue(m_gui_plane, "SCALING_FILTER", "Nearest Neighbor"))
  AddProperty(m_gui_plane, "SCALING_FILTER", "Nearest Neighbor")

You are asking for a specific value ("Nearest Neighbor") anyways, so there is no reason to traverse all the values. You could traverse all the values if you wanted to create a settingsfilter that will populate based on the enum values of "SCALING_FILTER", but that is only needed if you want to use other methods than just "Nearest Neighbor" and "Default".

[slattann]

@lrusak Please let me know if this is ok, I will incorporate.
I validated it and it works fine.
enum drm_scaling_filter { DRM_SCALING_FILTER_DEFAULT, DRM_SCALING_FILTER_NEAREST_NEIGHBOR, };
int32_t CDRMAtomic::GetScalingFilterType(const char* type) { if (!strcmp(type, "Nearest Neighbor")) return DRM_SCALING_FILTER_NEAREST_NEIGHBOR; else return DRM_SCALING_FILTER_DEFAULT; }
bool CDRMAtomic::SetScalingFilter(struct drm_object* object, const char* name, const char* type) { uint32_t iType = GetScalingFilterType(type); if (SupportsPropertyAndValue(object, name, iType)) if (AddProperty(object, name, iType)) { CLog::Log(LOGDEBUG, "CDRMAtomic::{}, Window size: {}x{}", __FUNCTION__, m_width, m_height); < . . . > return true; } return false; }

[lrusak]

This doesn't need to live here. Better to put it where it is used. (We have moved away from putting specific settings in CSettings)

[slattann]

Yeah this will be moved appropriately in using file.

[lrusak]

Same thing here.

[slattann]

I will take care.

[garbear]

Good job on the PR. Game dialog part looks perfect.

This is a really cool feature. Why not optimistically make it the default if support is detected?

[sarbes]

Nice work, I'll compile it for my N4100 laptop as soon as I have the time.

Just for my understanding, scaling non-square pixels (lets say a 4:3, 256x244 SNES game) would result in columns with uneven width?

[lrusak]

Nice work, I'll compile it for my N4100 laptop as soon as I have the time.

Just for my understanding, scaling non-square pixels (lets say a 4:3, 256x244 SNES game) would result in columns with uneven width?

@sarbes be aware that this is specific to kodi-gbm and requires the correct hardware that supports integer scaling (newer intel hardware) and possibly some kernel patches to expose the correct drm properties needed.

[slattann]

(newer intel hardware)

@sarbes you can refer these kernel patches: https://patchwork.freedesktop.org/series/73883/#rev6
and try on platforms like Icelake or newer hardware.

[slattann]

Updated the review comments with the changes, kindly review and let me know if changes needed my attention. Thanks

[lrusak]

indentation

[slattann]

Ok, I will fix.

[lrusak]

Overall you still need to run clang-format, there is a bunch of small formatting errors.

[ppaalanen]

Hi, a reminder just in case, hopefully this is not news to anyone:

This MR needs review acceptance so that the kernel patches can be merged, but this MR must NOT be merged until the Linux kernel patches have been merged into a non-rebasing upstream kernel tree.

For more information, see: https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements

[garbear]

Great! @lrusak any comments left?

[lrusak]

Great! @lrusak any comments left?

I'm ok with the changes. I don't love the changes to the drm system especially with the changes upcoming in #18491 but I can live with it and refactor the changes later.

@slattann what are the next steps? Do you need this to be approved before sending the patches for the kernel interface?

[slattann]

Great! @lrusak any comments left?

I'm ok with the changes. I don't love the changes to the drm system especially with the changes upcoming in #18491 but I can live with it and refactor the changes later.

@slattann what are the next steps? Do you need this to be approved before sending the patches for the kernel interface?

@lrusak It would be good if you give us MR acceptance, so that we can take that to Kernel changes to get merged. Following which you can help us to merge this PR.
Thanks and appreciate support from @garbear and you.

[ppaalanen]

@garbear Did you see the kernel patches land in a non-rebasing kernel git tree?

No-one commented here that they did, or where they landed.

If they have not landed yet, this MR should not have been merged! If the kernel patches have not landed, please make sure this feature does not end up in any release of XBMC in a way that it might be used without code changes.

For more information, see
https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements :

The kernel patch can only be merged after all the above requirements are met, but it must be merged to either drm-next or drm-misc-next before the userspace patches land.

I already mentioned this in #18194 (comment) in this very MR.

In the worst case, if you release XBMC with this feature enabled, and then kernel review comes up with changes to the UAPI, it means both the kernel patches and the XBMC patches have to be thrown away and re-written so that the kernel changes landing cannot break any version of XBMC that uses the wrong UAPI.

The reason I'm nitpicking about this is not that I think such a conflict is likely, but to educate userspace developers of the kernel requirements. Essentially you are part of the kernel review process here.

[garbear]

@ppaalanen I missed the desired order of merge. I propose we disable the feature by default, and open another PR enabling the feature that can be a stand-in for this PR. The new PR will have the hard requirement that it can't be merged until the kernel patches land.

This PR missed our Alpha 2 release. Alpha 3 isn't due until October 25 so we don't have to worry about this being enabled in a release prematurely.

@slattann what do you think about this strategy?

[lrusak]

I've added the commit to disable the feature in #18491 as 7f36caf

[slattann]

@garbear @lrusak @ppaalanen , Kernel patches are rebased again to the latest Linux-5.9.0-rc8+ and under review. Once they are merged we are good to make this into upcoming releases. What you guys say?

[garbear]

@lrusak Can you also default the setting to to false, maybe invisibilitalize it? (def. a word)

@slattann we're looking good on time. 2 weeks until an Alpha release, and it's just alpha. it'll be 3 weeks or more til beta. and we can enable whenever. we can easily work on kernel time at the moment

[ppaalanen]

Thank you very much for following the kernel procedure, kudos to @lrusak.

@slattann, just follow the kernel documentation on this, and make sure the patches are in the correct upstream branch before declaring them landed. Landing in arbitrary maintainer branches is not enough.

Thanks!

[emersion]

This incorrectly hard-codes the values of the DRM enum here.

The uAPI doesn't have this guarantee, e.g. "Nearest Neighbor" could be exposed as the integer 5 in some other driver.

[slattann]

We had discussed that point here before , In fact it was my initial implementation to read the DRM properties and set the proper value as per request. But later concluded on this implementation based on the discussions in this PR.

[emersion]

The suggested AddProperty(m_gui_plane, "SCALING_FILTER", "Nearest Neighbor") code is fine because it doesn't hardcode the value for "Nearest Neighbor". The code here in this PR is not fine.

[slattann]

The current AddProperty API does not take/pulls the valid values from DRM property, so went with the current implement with enums.
But if you think the code below make sense, we can incorporate the same instead.
bool CDRMAtomic::AddPropertyValueByName(struct drm_object* object, const char* name, const char* type) { for (uint32_t i = 0; i < object->props->count_props; i++) { if (!strcmp(object->props_info[i]->name, name)) { for (uint32_t j = 0; j < object->props_info[i]->count_enums; j++) { if (!strcmp(object->props_info[i]->enums[j].name, type)) { if (drmModeAtomicAddProperty(m_req, object->id, property_id, m_gui_plane->props_info[i]->enums[j].value) < 0) { CLog::Log(LOGERROR, "CDRMAtomic::%s - could not add property %s", __FUNCTION__, name); return false; } } } } } return true; }
This was the exact implementation which I had before.
Please let me know your thought on the same.

[emersion]

This code is very hard to read, please put it inside triple-backquotes like so:

```cpp
<insert code here>
```

[lrusak]

No, the enum value is not in the uAPI. The enum value is exposed by the kernel at runtime via drmModePropertyRes. The enum value may change from one driver to another, and from one plane to another.

Only the enum name (the string) is in the uAPI.

That's no problem, we evaluate all properties for a drm object at startup. So if we need to expand the check or add another one that's fine.

[emersion]

The issue is this enum. The value of DRM_SCALING_FILTER_NEAREST_NEIGHBOR is hard-coded to 1, returned from CDRMAtomic::GetScalingFilterType, then passed as-is to AddProperty in CDRMAtomic::SetScalingFilter.

[lrusak]

The issue is this enum. The value of DRM_SCALING_FILTER_NEAREST_NEIGHBOR is hard-coded to 1, returned from CDRMAtomic::GetScalingFilterType, then passed as-is to AddProperty in CDRMAtomic::SetScalingFilter.

it can be fixed 😺

[ppaalanen]

It must be fixed. @emersion is right.

This is actually one of those kinds of bugs that could lead to problems for kernel developers later, if they have to work around broken userspace assumptions to keep broken userspace working.

The general rule indeed is that KMS property enum values MUST be recognized by their string names, not by their numeric values. Unfortunately the KMS UAPI has past mistakes where the numerical values are already in the UAPI headers - that cannot be fixed, but all new KMS properties need to follow the rules, hence userspace can't pick an enum value by number. That is the reason you also cannot find the numbers in the kernel/libdrm UAPI headers.

[slattann]

I have created another PR for the fix: #18567
Kindly let me know if that is fine and we can go ahead w/ this PR and get Kernel patches reviewed and merged ?

[slattann]

Thank you very much for following the kernel procedure, kudos to @lrusak.

@slattann, just follow the kernel documentation on this, and make sure the patches are in the correct upstream branch before declaring them landed. Landing in arbitrary maintainer branches is not enough.

Thanks!

Yes @ppaalanen , we are planning to merge into drm-tip.

Thanks for bringing in discipline to merge both Kernel & user space changes for the feature.
@garbear & @lrusak , Thanks for your support throughout.