Re: [Freedreno] [PATCH v2 2/2] drm/msm: Cleanup A6XX opp-level reading
Hi Douglas,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v5.0-rc2]
[also build test ERROR on next-20190116]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Douglas-Anderson/drm-msm-Fix-A6XX-support-for-opp-level/20190118-042538
config: arm-imx_v6_v7_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 8.2.0-11) 8.2.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.2.0 make.cross ARCH=arm
All errors (new ones prefixed by >>):
drivers/gpu/drm/msm/adreno/a6xx_gmu.c: In function 'a6xx_gmu_get_arc_level':
>> drivers/gpu/drm/msm/adreno/a6xx_gmu.c:944:8: error: implicit declaration of
>> function 'dev_pm_opp_get_level'; did you mean 'dev_pm_opp_get_freq'?
>> [-Werror=implicit-function-declaration]
val = dev_pm_opp_get_level(opp);
^~~~
dev_pm_opp_get_freq
cc1: some warnings being treated as errors
vim +944 drivers/gpu/drm/msm/adreno/a6xx_gmu.c
929
930 /* Return the 'arc-level' for the given frequency */
931 static unsigned int a6xx_gmu_get_arc_level(struct device *dev,
932 unsigned long freq)
933 {
934 struct dev_pm_opp *opp;
935 unsigned int val;
936
937 if (!freq)
938 return 0;
939
940 opp = dev_pm_opp_find_freq_exact(dev, freq, true);
941 if (IS_ERR(opp))
942 return 0;
943
> 944 val = dev_pm_opp_get_level(opp);
945
946 dev_pm_opp_put(opp);
947
948 return val;
949 }
950
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH] drm: Split out drm_probe_helper.h
Hi Daniel et al. > > > > Yeah the drm_crtc_helper.h header is a bit the miniature drmP.h for legacy > > kms drivers. Just removing it from all the atomic drivers caused lots of > > fallout, I expect even more if you entirely remove the includes it has. > > Maybe a todo, care to pls create that patch since it's your idea? > > The main reason I bailed out initially was that this would create > small changes to several otherwise seldomly touched files. > And then we would later come and remove drmP.h - so lots of > small but incremental changes to the same otherwise seldomly > edited files. > And the job was only partially done. > > I will try to experiment with an approach where I clean up the > include/drm/*.h files a little (like suggested above, +delete drmP.h > and maybe a bit more). > > Then to try on a driver by driver basis to make it build with a > cleaned set of include files. > I hope that the cleaned up driver can still build without the > cleaned header files so the changes can be submitted piecemal. > > Will do so with an eye on the lesser maintained drivers to try it > out to avoid creating too much chrunch for others. I have now a few patches queued, but the result is not too pretty. I did the following: - For all files in include/drm/*.h the set of include files were adjusted to the minimum number of files required to make them build without any other files included first. Created one .c file for each .h file. Then included the .h file and adjusted to the minimal set of include files. In the process a lot of forwards were added. - Deleted drmP.h - Fixed build of a few drivers: sti, tilcdc, gma500, tve200, via Some observations: - Killing all the includes not needed in the headers files results in a a lot of extra changes. Examples: drm_modseset_helper_vtables.h is no longer included by anyone, so needs to be added in many files drm_atomic_state_helper.h is no longer included by anyone so likewise needs to be added in many files - It is very tedious to do this properly. The process I followed was: - delete / comment out all include files - add back the obvious from a quick scan of the code - build - fix - build - fix - build - fix ... - next file... - The result is errorprone as only the allyesconfig + allmodconfig variants are tested. But reallife configurations are more diverse. Current diffstat: 111 files changed, 771 insertions(+), 401 deletions(-) This is for the 5 drivers alone and not the header cleanup. So long story short - this is not good and not the way forward. I will try to come up with a few improvements to make the headers files selfcontained, but restricted to the changes that add forwards/include to avoid the chrunch in all the drivers. And then post for review a few patches to clean up some headers. If the cleanup gets a go I will try to persuade the introduction of these. This will include, but will not be limited to, the above mentioned drm_crtc_helper.h header file. For now too much time was already spent on this, so it is at the moment pushed back on my TODO list. This mail serve also as a kind of "where had I left", when/if I pick this up again. If there are anyone that knows some tooling that can help in the process of adjusting the header files I am all ears. Sam ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3 1/3] drm/msm/a6xx: Add support for an interconnect path
Hi Rob,
On 1/18/19 21:16, Rob Clark wrote:
> On Fri, Jan 18, 2019 at 1:06 PM Doug Anderson wrote:
>>
>> Hi,
>>
>> On Thu, Dec 20, 2018 at 9:30 AM Jordan Crouse wrote:
>>>
>>> Try to get the interconnect path for the GPU and vote for the maximum
>>> bandwidth to support all frequencies. This is needed for performance.
>>> Later we will want to scale the bandwidth based on the frequency to
>>> also optimize for power but that will require some device tree
>>> infrastructure that does not yet exist.
>>>
>>> v5: Remove hardcoded interconnect name and just use the default
>>
>> nit: ${SUBJECT} says v3, but this is v5.
>>
>> I'll put in my usual plug for considering "patman" to help post
>> patches. Even though it lives in the u-boot git repo it's still a gem
>> for kernel work.
>>
>>
>>
>>> @@ -85,6 +89,12 @@ static void __a6xx_gmu_set_freq(struct a6xx_gmu *gmu,
>>> int index)
>>> dev_err(gmu->dev, "GMU set GPU frequency error: %d\n", ret);
>>>
>>> gmu->freq = gmu->gpu_freqs[index];
>>> +
>>> + /*
>>> +* Eventually we will want to scale the path vote with the
>>> frequency but
>>> +* for now leave it at max so that the performance is nominal.
>>> +*/
>>> + icc_set(gpu->icc_path, 0, MBps_to_icc(7216));
>>
>> You'll need to change icc_set() here to icc_set_bw() to match v13, AKA:
>>
>> - https://patchwork.kernel.org/patch/10766335/
>> - https://lkml.kernel.org/r/20190116161103.6937-2-georgi.dja...@linaro.org
>>
>>
>>> @@ -695,6 +707,9 @@ int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu)
>>> if (ret)
>>> goto out;
>>>
>>> + /* Set the bus quota to a reasonable value for boot */
>>> + icc_set(gpu->icc_path, 0, MBps_to_icc(3072));
>>
>> This will also need to change to icc_set_bw()
>>
>>
>>> @@ -781,6 +798,9 @@ int a6xx_gmu_stop(struct a6xx_gpu *a6xx_gpu)
>>> /* Tell RPMh to power off the GPU */
>>> a6xx_rpmh_stop(gmu);
>>>
>>> + /* Remove the bus vote */
>>> + icc_set(gpu->icc_path, 0, 0);
>>
>> This will also need to change to icc_set_bw()
>>
>>
>> I have the same questions for this series that I had in response to
>> the email ("[v5 2/3] drm/msm/dpu: Integrate interconnect API in MDSS")
>>
>>
>>
>> Copy / pasting here (with minor name changes) so folks don't have to
>> follow links / search email.
>>
>> ==
>>
>> I'm curious what the plan is for landing this series. Rob / Gerogi:
>> do you have any preference? Options I'd imagine:
>>
>> A) Wait until interconnect lands (in 5.1?) and land this through
>> msm-next in the version after (5.2?)
>>
>> B) Georgi provides an immutable branch for interconnect when his lands
>> (assuming he's landing via pull request) and that gets pulled into the
>> the relevant drm tree.
>>
>> C) Rob Acks this series and indicates that it should go in through
>> Gerogi's tree (probably only works if Georgi plans to send a pull
>> request). If we're going this route then (IIUC) we'd want to land
>> this in Gerogi's tree sooner rather than later so it can get some bake
>> time? NOTE: as per my prior reply, I believe Rob has already Acked
>> this patch.
>>
>
> I'm ok to ack and have it land via Georgi's tree, if Georgi wants to
> do that. Or otherwise, I could maybe coordinate w/ airlied to send a
> 2nd late msm-next pr including the gpu and display interconnect
> patches.
I'm fine either way. But it would be nice if both patches (this one and
the dt-bindings go together. The v6 of this patch applies cleanly to my
tree, but the next one (2/3) with the dt-bindings doesn't.
Thanks,
Georgi
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 2/2] drm/msm: Use DRM_DEV_INFO_RATELIMITED for shrinker messages
On Fri, 18 Jan 2019, "Kristian H. Kristensen" wrote:
> Otherwise we get hard to track down "Purging: 123123 bytes" messages in
> the log.
>
> Signed-off-by: Kristian H. Kristensen
> ---
> drivers/gpu/drm/msm/msm_gem_shrinker.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c
> b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> index b72d8e6cd51d7..8161923892f55 100644
> --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
> +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> @@ -98,7 +98,7 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct
> shrink_control *sc)
> mutex_unlock(>struct_mutex);
>
> if (freed > 0)
> - pr_info_ratelimited("Purging %lu bytes\n", freed << PAGE_SHIFT);
> + DRM_DEV_INFO_RATELIMITED(dev->dev, "Purging %lu bytes\n", freed
> << PAGE_SHIFT);
I'm not opposed to the patches per se, but it does seem a bit odd to be
printing info level messages in a way that might need ratelimiting. Is
this a hint you should perhaps make it debug?
BR,
Jani.
>
> return freed;
> }
> @@ -134,7 +134,7 @@ msm_gem_shrinker_vmap(struct notifier_block *nb, unsigned
> long event, void *ptr)
> *(unsigned long *)ptr += unmapped;
>
> if (unmapped > 0)
> - pr_info_ratelimited("Purging %u vmaps\n", unmapped);
> + DRM_DEV_INFO_RATELIMITED(dev->dev, "Purging %u vmaps\n",
> unmapped);
>
> return NOTIFY_DONE;
> }
--
Jani Nikula, Intel Open Source Graphics Center
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
