Re: [PATCH 08/11] drm/msm/dp: switch to struct drm_edid
On Sun, 19 May 2024, Dmitry Baryshkov wrote:
> On Tue, May 14, 2024 at 03:55:14PM +0300, Jani Nikula wrote:
>> Prefer the struct drm_edid based functions for reading the EDID and
>> updating the connector.
>>
>> Simplify the flow by updating the EDID property when the EDID is read
>> instead of at .get_modes.
>>
>> Signed-off-by: Jani Nikula
>>
>> ---
>
> The patch looks good to me, I'd like to hear an opinion from Doug (added
> to CC).
>
> Reviewed-by: Dmitry Baryshkov
Thanks!
> What is the merge strategy for the series? Do you plan to pick up all
> the patches to drm-misc or should we pick up individual patches into
> driver trees?
I think all of the patches here are connected in theme, but
independent. Either way is fine by me.
>
>
>>
>> Cc: Rob Clark
>> Cc: Abhinav Kumar
>> Cc: Dmitry Baryshkov
>> Cc: Sean Paul
>> Cc: Marijn Suijten
>> Cc: linux-arm-...@vger.kernel.org
>> Cc: freedreno@lists.freedesktop.org
>> ---
>> drivers/gpu/drm/msm/dp/dp_display.c | 11 +++
>> drivers/gpu/drm/msm/dp/dp_panel.c | 47 +
>> drivers/gpu/drm/msm/dp/dp_panel.h | 2 +-
>> 3 files changed, 20 insertions(+), 40 deletions(-)
>
> [skipped]
>
>> @@ -249,10 +228,12 @@ void dp_panel_handle_sink_request(struct dp_panel
>> *dp_panel)
>> panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
>>
>> if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
>> +/* FIXME: get rid of drm_edid_raw() */
>
> The code here can get use of something like drm_edid_smth_checksum().
> 'Something', because I could not come up with the word that would make
> it clear that it is the declared checksum instead of the actual /
> computed one.
This is an annoying one, to be honest, and linked to the historical fact
that we filter some EDID blocks that have an incorrect checksum.
(Some blocks, yes. We don't filter all blocks, because there are some
nasty docks out there that modify the CTA block but fail to update the
checksum, and filtering the CTA blocks would render the display
useless. So we accept CTA blocks with incorrect checksums. But reject
others. Yay.)
IMO the real fix would be to stop mucking with the EDID, and just expose
it to userspace, warts and all. We could still ignore the EDID blocks
with incorrect checksum while using it ourselves if we want to. And with
that, we could just have a function that checks the last EDID block's
checksum, and stop using this ->real_edid_checksum thing.
Anyway, yes, we could add the function already.
BR,
Jani.
>
>> +const struct edid *edid = drm_edid_raw(dp_panel->drm_edid);
>> u8 checksum;
>>
>> -if (dp_panel->edid)
>> - checksum = dp_panel_get_edid_checksum(dp_panel->edid);
>> +if (edid)
>> +checksum = dp_panel_get_edid_checksum(edid);
>> else
>> checksum = dp_panel->connector->real_edid_checksum;
>>
--
Jani Nikula, Intel
[PATCH 08/11] drm/msm/dp: switch to struct drm_edid
Prefer the struct drm_edid based functions for reading the EDID and
updating the connector.
Simplify the flow by updating the EDID property when the EDID is read
instead of at .get_modes.
Signed-off-by: Jani Nikula
---
Cc: Rob Clark
Cc: Abhinav Kumar
Cc: Dmitry Baryshkov
Cc: Sean Paul
Cc: Marijn Suijten
Cc: linux-arm-...@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
---
drivers/gpu/drm/msm/dp/dp_display.c | 11 +++
drivers/gpu/drm/msm/dp/dp_panel.c | 47 +
drivers/gpu/drm/msm/dp/dp_panel.h | 2 +-
3 files changed, 20 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 672a7ba52eda..9622e58dce3e 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -360,26 +360,25 @@ static int dp_display_send_hpd_notification(struct
dp_display_private *dp,
static int dp_display_process_hpd_high(struct dp_display_private *dp)
{
+ struct drm_connector *connector = dp->dp_display.connector;
+ const struct drm_display_info *info = >display_info;
int rc = 0;
- struct edid *edid;
- rc = dp_panel_read_sink_caps(dp->panel, dp->dp_display.connector);
+ rc = dp_panel_read_sink_caps(dp->panel, connector);
if (rc)
goto end;
dp_link_process_request(dp->link);
if (!dp->dp_display.is_edp)
- drm_dp_set_subconnector_property(dp->dp_display.connector,
+ drm_dp_set_subconnector_property(connector,
connector_status_connected,
dp->panel->dpcd,
dp->panel->downstream_ports);
- edid = dp->panel->edid;
-
dp->dp_display.psr_supported = dp->panel->psr_cap.version &&
psr_enabled;
- dp->audio_supported = drm_detect_monitor_audio(edid);
+ dp->audio_supported = info->has_audio;
dp_panel_handle_sink_request(dp->panel);
/*
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
b/drivers/gpu/drm/msm/dp/dp_panel.c
index 07db8f37cd06..a916b5f3b317 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -108,28 +108,6 @@ static u32 dp_panel_get_supported_bpp(struct dp_panel
*dp_panel,
return bpp;
}
-static int dp_panel_update_modes(struct drm_connector *connector,
- struct edid *edid)
-{
- int rc = 0;
-
- if (edid) {
- rc = drm_connector_update_edid_property(connector, edid);
- if (rc) {
- DRM_ERROR("failed to update edid property %d\n", rc);
- return rc;
- }
- rc = drm_add_edid_modes(connector, edid);
- return rc;
- }
-
- rc = drm_connector_update_edid_property(connector, NULL);
- if (rc)
- DRM_ERROR("failed to update edid property %d\n", rc);
-
- return rc;
-}
-
int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
struct drm_connector *connector)
{
@@ -175,12 +153,13 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel,
if (rc)
return rc;
- kfree(dp_panel->edid);
- dp_panel->edid = NULL;
+ drm_edid_free(dp_panel->drm_edid);
+
+ dp_panel->drm_edid = drm_edid_read_ddc(connector, >aux->ddc);
+
+ drm_edid_connector_update(connector, dp_panel->drm_edid);
- dp_panel->edid = drm_get_edid(connector,
- >aux->ddc);
- if (!dp_panel->edid) {
+ if (!dp_panel->drm_edid) {
DRM_ERROR("panel edid read failed\n");
/* check edid read fail is due to unplug */
if (!dp_catalog_link_is_connected(panel->catalog)) {
@@ -224,13 +203,13 @@ int dp_panel_get_modes(struct dp_panel *dp_panel,
return -EINVAL;
}
- if (dp_panel->edid)
- return dp_panel_update_modes(connector, dp_panel->edid);
+ if (dp_panel->drm_edid)
+ return drm_edid_connector_add_modes(connector);
return 0;
}
-static u8 dp_panel_get_edid_checksum(struct edid *edid)
+static u8 dp_panel_get_edid_checksum(const struct edid *edid)
{
edid += edid->extensions;
@@ -249,10 +228,12 @@ void dp_panel_handle_sink_request(struct dp_panel
*dp_panel)
panel = container_of(dp_panel, struct dp_panel_private, dp_panel);
if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) {
+ /* FIXME: get rid of drm_edid_raw() */
+ const struct edid *edid = drm_edid_raw(dp_panel->drm_edid);
u8 checksum;
- if (dp_panel->edid)
- ch
Re: [PATCH] docs: document python version used for compilation
On Fri, 10 May 2024, Mauro Carvalho Chehab wrote: > Em Fri, 10 May 2024 11:08:38 +0300 > Jani Nikula escreveu: > >> On Thu, 09 May 2024, Dmitry Baryshkov wrote: >> > The drm/msm driver had adopted using Python3 script to generate register >> > header files instead of shipping pre-generated header files. Document >> > the minimal Python version supported by the script. >> > >> > Signed-off-by: Dmitry Baryshkov >> > --- >> > Documentation/process/changes.rst | 1 + >> > 1 file changed, 1 insertion(+) >> > >> > diff --git a/Documentation/process/changes.rst >> > b/Documentation/process/changes.rst >> > index 5685d7bfe4d0..8d225a9f65a2 100644 >> > --- a/Documentation/process/changes.rst >> > +++ b/Documentation/process/changes.rst >> > @@ -63,6 +63,7 @@ cpio any cpio --version >> > GNU tar1.28 tar --version >> > gtags (optional) 6.6.5gtags --version >> > mkimage (optional) 2017.01 mkimage --version >> > +Python (optional) 3.5.xpython3 --version >> >> Python 3.5 reached end-of-life 3½ years ago [1]. What's the point in >> using anything older than the oldest supported version of Python, >> i.e. 3.8 at this time? > > What's the point of breaking compilation with on older distros? > The idea of minimal versions here is to specify the absolute minimum > version that it is required for the build to happen. If 3.5 is > the minimal one, then be it. AFAICT 3.5 was an arbitrary rather than a deliberate choice. We should at least be aware *why* we'd be sticking to old versions. Minimum versions here also means sticking to features available in said versions, for Python just as well as for GCC or any other tool. That's not zero cost. I guess there are two angles here too. The absolute minimum version currently required, and the, uh, maximum the minimum version can be safely bumped to. Say, you want to use a feature not available in the current minimum, how far up can you bump the version to? Could we define and document the criteria (e.g. based on distros as you suggest below) so we don't have to repeat the discussion? BR, Jani. > > - > > Now, a criteria is needed to raise the minimal version. IMO, the > minimal version shall be at least the minimal one present on most > used LTS distros that are not EOL. > > I would look for at least 4 such distros: > > - Debian > > Looking at https://wiki.debian.org/LTS, Debian 10 EOL will be on > June, 2024. > > Looking at: > > https://distrowatch.com/table.php?distribution=debian > > Debian 10 uses python 3.7.3. > > - Looking at Distrowatch for openSUSE Leap 15.5, it uses Python > 3.6.15 and has an EOL schedule for Dec, 2024. > > - RHEL 8.9 uses a bigger version than those two - 3.11.5 - again > looking at Distrowatch to check it. > > - SLES 15 SP4 and above uses Python 3.11, according with: > https://www.suse.com/c/python-3-11-stack-for-suse-linux-enterprise-15/ > > From the above, IMO kernel shall support building with Python 3.6 > at least until the end of this year. > > Regards, > Mauro -- Jani Nikula, Intel
Re: [PATCH] docs: document python version used for compilation
On Thu, 09 May 2024, Dmitry Baryshkov wrote: > The drm/msm driver had adopted using Python3 script to generate register > header files instead of shipping pre-generated header files. Document > the minimal Python version supported by the script. > > Signed-off-by: Dmitry Baryshkov > --- > Documentation/process/changes.rst | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/process/changes.rst > b/Documentation/process/changes.rst > index 5685d7bfe4d0..8d225a9f65a2 100644 > --- a/Documentation/process/changes.rst > +++ b/Documentation/process/changes.rst > @@ -63,6 +63,7 @@ cpio any cpio --version > GNU tar1.28 tar --version > gtags (optional) 6.6.5gtags --version > mkimage (optional) 2017.01 mkimage --version > +Python (optional) 3.5.xpython3 --version Python 3.5 reached end-of-life 3½ years ago [1]. What's the point in using anything older than the oldest supported version of Python, i.e. 3.8 at this time? BR, Jani. [1] https://devguide.python.org/versions/ > == === > > > .. [#f1] Sphinx is needed only to build the Kernel documentation > > --- > base-commit: 704ba27ac55579704ba1289392448b0c66b56258 > change-id: 20240509-python-version-a8b6ca2125ff > > Best regards, -- Jani Nikula, Intel
Re: [PATCH] drm/msm: remove python 3.9 dependency for compiling msm
On Tue, 07 May 2024, Abhinav Kumar wrote:
> Since commit 5acf49119630 ("drm/msm: import gen_header.py script from Mesa"),
> compilation is broken on machines having python versions older than 3.9
> due to dependency on argparse.BooleanOptionalAction.
Is it now okay to require Python for the build? Not listed in
Documentation/process/changes.rst.
BR,
Jani.
>
> Switch to use simple bool for the validate flag to remove the dependency.
>
> Fixes: 5acf49119630 ("drm/msm: import gen_header.py script from Mesa")
> Signed-off-by: Abhinav Kumar
> ---
> drivers/gpu/drm/msm/registers/gen_header.py | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/registers/gen_header.py
> b/drivers/gpu/drm/msm/registers/gen_header.py
> index fc3bfdc991d2..3926485bb197 100644
> --- a/drivers/gpu/drm/msm/registers/gen_header.py
> +++ b/drivers/gpu/drm/msm/registers/gen_header.py
> @@ -538,7 +538,7 @@ class Parser(object):
> self.variants.add(reg.domain)
>
> def do_validate(self, schemafile):
> - if self.validate == False:
> + if not self.validate:
> return
>
> try:
> @@ -948,7 +948,8 @@ def main():
> parser = argparse.ArgumentParser()
> parser.add_argument('--rnn', type=str, required=True)
> parser.add_argument('--xml', type=str, required=True)
> - parser.add_argument('--validate', action=argparse.BooleanOptionalAction)
> + parser.add_argument('--validate', default=False, action='store_true')
> + parser.add_argument('--no-validate', dest='validate',
> action='store_false')
>
> subparsers = parser.add_subparsers()
> subparsers.required = True
--
Jani Nikula, Intel
Re: [PATCH 1/2] drm/print: drop include debugfs.h and include where needed
On Mon, 22 Apr 2024, Jani Nikula wrote: > Surprisingly many places depend on debugfs.h to be included via > drm_print.h. Fix them. > > v3: Also fix armada, ite-it6505, imagination, msm, sti, vc4, and xe > > v2: Also fix ivpu and vmwgfx > > Reviewed-by: Andrzej Hajda > Acked-by: Maxime Ripard > Link: > https://patchwork.freedesktop.org/patch/msgid/20240410141434.157908-1-jani.nik...@intel.com > Signed-off-by: Jani Nikula While the changes all over the place are small, mostly just adding the debugfs.h include, please consider acking. I've sent this a few times already. Otherwise, I'll merge this by the end of the week, acks or not. Thanks, Jani. > > --- > > Cc: Jacek Lawrynowicz > Cc: Stanislaw Gruszka > Cc: Oded Gabbay > Cc: Russell King > Cc: David Airlie > Cc: Daniel Vetter > Cc: Andrzej Hajda > Cc: Neil Armstrong > Cc: Robert Foss > Cc: Laurent Pinchart > Cc: Jonas Karlman > Cc: Jernej Skrabec > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: Jani Nikula > Cc: Rodrigo Vivi > Cc: Joonas Lahtinen > Cc: Tvrtko Ursulin > Cc: Frank Binns > Cc: Matt Coster > Cc: Rob Clark > Cc: Abhinav Kumar > Cc: Dmitry Baryshkov > Cc: Sean Paul > Cc: Marijn Suijten > Cc: Karol Herbst > Cc: Lyude Paul > Cc: Danilo Krummrich > Cc: Alex Deucher > Cc: "Christian König" > Cc: "Pan, Xinhui" > Cc: Alain Volmat > Cc: Huang Rui > Cc: Zack Rusin > Cc: Broadcom internal kernel review list > > Cc: Lucas De Marchi > Cc: "Thomas Hellström" > Cc: dri-de...@lists.freedesktop.org > Cc: intel-...@lists.freedesktop.org > Cc: intel...@lists.freedesktop.org > Cc: linux-arm-...@vger.kernel.org > Cc: freedreno@lists.freedesktop.org > Cc: nouv...@lists.freedesktop.org > Cc: amd-...@lists.freedesktop.org > --- > drivers/accel/ivpu/ivpu_debugfs.c | 2 ++ > drivers/gpu/drm/armada/armada_debugfs.c | 1 + > drivers/gpu/drm/bridge/ite-it6505.c | 1 + > drivers/gpu/drm/bridge/panel.c | 2 ++ > drivers/gpu/drm/drm_print.c | 6 +++--- > drivers/gpu/drm/i915/display/intel_dmc.c| 1 + > drivers/gpu/drm/imagination/pvr_fw_trace.c | 1 + > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 2 ++ > drivers/gpu/drm/nouveau/dispnv50/crc.c | 2 ++ > drivers/gpu/drm/radeon/r100.c | 1 + > drivers/gpu/drm/radeon/r300.c | 1 + > drivers/gpu/drm/radeon/r420.c | 1 + > drivers/gpu/drm/radeon/r600.c | 3 ++- > drivers/gpu/drm/radeon/radeon_fence.c | 1 + > drivers/gpu/drm/radeon/radeon_gem.c | 1 + > drivers/gpu/drm/radeon/radeon_ib.c | 2 ++ > drivers/gpu/drm/radeon/radeon_pm.c | 1 + > drivers/gpu/drm/radeon/radeon_ring.c| 2 ++ > drivers/gpu/drm/radeon/radeon_ttm.c | 1 + > drivers/gpu/drm/radeon/rs400.c | 1 + > drivers/gpu/drm/radeon/rv515.c | 1 + > drivers/gpu/drm/sti/sti_drv.c | 1 + > drivers/gpu/drm/ttm/ttm_device.c| 1 + > drivers/gpu/drm/ttm/ttm_resource.c | 3 ++- > drivers/gpu/drm/ttm/ttm_tt.c| 5 +++-- > drivers/gpu/drm/vc4/vc4_drv.h | 1 + > drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 2 ++ > drivers/gpu/drm/xe/xe_debugfs.c | 1 + > drivers/gpu/drm/xe/xe_gt_debugfs.c | 2 ++ > drivers/gpu/drm/xe/xe_uc_debugfs.c | 2 ++ > include/drm/drm_print.h | 2 +- > 31 files changed, 46 insertions(+), 8 deletions(-) > > diff --git a/drivers/accel/ivpu/ivpu_debugfs.c > b/drivers/accel/ivpu/ivpu_debugfs.c > index d09d29775b3f..e07e447d08d1 100644 > --- a/drivers/accel/ivpu/ivpu_debugfs.c > +++ b/drivers/accel/ivpu/ivpu_debugfs.c > @@ -3,6 +3,8 @@ > * Copyright (C) 2020-2023 Intel Corporation > */ > > +#include > + > #include > #include > #include > diff --git a/drivers/gpu/drm/armada/armada_debugfs.c > b/drivers/gpu/drm/armada/armada_debugfs.c > index 29f4b52e3c8d..a763349dd89f 100644 > --- a/drivers/gpu/drm/armada/armada_debugfs.c > +++ b/drivers/gpu/drm/armada/armada_debugfs.c > @@ -5,6 +5,7 @@ > */ > > #include > +#include > #include > #include > #include > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c > b/drivers/gpu/drm/bridge/ite-it6505.c > index 27334173e911..3f68c82888c2 100644 > --- a/drivers/gpu/drm/bridge/ite-it6505.c > +++ b/drivers/gpu/drm/bridge/ite-it6505.c > @@ -3,6 +3,7 @@ > * Copyright (c) 2020, The Linux Foundation. All rights reserved. > */ > #include > +#include > #include > #include > #includ
[PATCH 1/2] drm/print: drop include debugfs.h and include where needed
Surprisingly many places depend on debugfs.h to be included via drm_print.h. Fix them. v3: Also fix armada, ite-it6505, imagination, msm, sti, vc4, and xe v2: Also fix ivpu and vmwgfx Reviewed-by: Andrzej Hajda Acked-by: Maxime Ripard Link: https://patchwork.freedesktop.org/patch/msgid/20240410141434.157908-1-jani.nik...@intel.com Signed-off-by: Jani Nikula --- Cc: Jacek Lawrynowicz Cc: Stanislaw Gruszka Cc: Oded Gabbay Cc: Russell King Cc: David Airlie Cc: Daniel Vetter Cc: Andrzej Hajda Cc: Neil Armstrong Cc: Robert Foss Cc: Laurent Pinchart Cc: Jonas Karlman Cc: Jernej Skrabec Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: Jani Nikula Cc: Rodrigo Vivi Cc: Joonas Lahtinen Cc: Tvrtko Ursulin Cc: Frank Binns Cc: Matt Coster Cc: Rob Clark Cc: Abhinav Kumar Cc: Dmitry Baryshkov Cc: Sean Paul Cc: Marijn Suijten Cc: Karol Herbst Cc: Lyude Paul Cc: Danilo Krummrich Cc: Alex Deucher Cc: "Christian König" Cc: "Pan, Xinhui" Cc: Alain Volmat Cc: Huang Rui Cc: Zack Rusin Cc: Broadcom internal kernel review list Cc: Lucas De Marchi Cc: "Thomas Hellström" Cc: dri-de...@lists.freedesktop.org Cc: intel-...@lists.freedesktop.org Cc: intel...@lists.freedesktop.org Cc: linux-arm-...@vger.kernel.org Cc: freedreno@lists.freedesktop.org Cc: nouv...@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- drivers/accel/ivpu/ivpu_debugfs.c | 2 ++ drivers/gpu/drm/armada/armada_debugfs.c | 1 + drivers/gpu/drm/bridge/ite-it6505.c | 1 + drivers/gpu/drm/bridge/panel.c | 2 ++ drivers/gpu/drm/drm_print.c | 6 +++--- drivers/gpu/drm/i915/display/intel_dmc.c| 1 + drivers/gpu/drm/imagination/pvr_fw_trace.c | 1 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 2 ++ drivers/gpu/drm/nouveau/dispnv50/crc.c | 2 ++ drivers/gpu/drm/radeon/r100.c | 1 + drivers/gpu/drm/radeon/r300.c | 1 + drivers/gpu/drm/radeon/r420.c | 1 + drivers/gpu/drm/radeon/r600.c | 3 ++- drivers/gpu/drm/radeon/radeon_fence.c | 1 + drivers/gpu/drm/radeon/radeon_gem.c | 1 + drivers/gpu/drm/radeon/radeon_ib.c | 2 ++ drivers/gpu/drm/radeon/radeon_pm.c | 1 + drivers/gpu/drm/radeon/radeon_ring.c| 2 ++ drivers/gpu/drm/radeon/radeon_ttm.c | 1 + drivers/gpu/drm/radeon/rs400.c | 1 + drivers/gpu/drm/radeon/rv515.c | 1 + drivers/gpu/drm/sti/sti_drv.c | 1 + drivers/gpu/drm/ttm/ttm_device.c| 1 + drivers/gpu/drm/ttm/ttm_resource.c | 3 ++- drivers/gpu/drm/ttm/ttm_tt.c| 5 +++-- drivers/gpu/drm/vc4/vc4_drv.h | 1 + drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 2 ++ drivers/gpu/drm/xe/xe_debugfs.c | 1 + drivers/gpu/drm/xe/xe_gt_debugfs.c | 2 ++ drivers/gpu/drm/xe/xe_uc_debugfs.c | 2 ++ include/drm/drm_print.h | 2 +- 31 files changed, 46 insertions(+), 8 deletions(-) diff --git a/drivers/accel/ivpu/ivpu_debugfs.c b/drivers/accel/ivpu/ivpu_debugfs.c index d09d29775b3f..e07e447d08d1 100644 --- a/drivers/accel/ivpu/ivpu_debugfs.c +++ b/drivers/accel/ivpu/ivpu_debugfs.c @@ -3,6 +3,8 @@ * Copyright (C) 2020-2023 Intel Corporation */ +#include + #include #include #include diff --git a/drivers/gpu/drm/armada/armada_debugfs.c b/drivers/gpu/drm/armada/armada_debugfs.c index 29f4b52e3c8d..a763349dd89f 100644 --- a/drivers/gpu/drm/armada/armada_debugfs.c +++ b/drivers/gpu/drm/armada/armada_debugfs.c @@ -5,6 +5,7 @@ */ #include +#include #include #include #include diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c index 27334173e911..3f68c82888c2 100644 --- a/drivers/gpu/drm/bridge/ite-it6505.c +++ b/drivers/gpu/drm/bridge/ite-it6505.c @@ -3,6 +3,7 @@ * Copyright (c) 2020, The Linux Foundation. All rights reserved. */ #include +#include #include #include #include diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 7f41525f7a6e..32506524d9a2 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -4,6 +4,8 @@ * Copyright (C) 2017 Broadcom */ +#include + #include #include #include diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c index 699b7dbffd7b..cf2efb44722c 100644 --- a/drivers/gpu/drm/drm_print.c +++ b/drivers/gpu/drm/drm_print.c @@ -23,13 +23,13 @@ * Rob Clark */ -#include - +#include +#include #include #include #include #include -#include +#include #include #include diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c index 3697a02b51b6..09346afd1f0e 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc.c +++ b/drivers/gpu/drm/i915/display/intel_dmc.c @@ -22,6 +22,7 @@ * */ +#include #include #include "i915_drv.h"
Re: [PATCH] drm/msm: convert all pixel format logging to use %p4cc
On Mon, 08 Apr 2024, Dmitry Baryshkov wrote: > On Mon, 8 Apr 2024 at 11:09, Jani Nikula wrote: >> Thanks! Do you take this via the msm tree? > > Yes, I will Forgot to mention, there's a Tested-by at [1]. Tested-by: Aishwarya TCV [1] https://lore.kernel.org/r/157e69d4-850f-40d2-84ff-d25dd2c12...@arm.com -- Jani Nikula, Intel
Re: [PATCH] drm/msm: convert all pixel format logging to use %p4cc
On Fri, 05 Apr 2024, Dmitry Baryshkov wrote: > On Fri, Apr 05, 2024 at 12:29:07PM +0300, Jani Nikula wrote: >> Logging u32 pixel formats using %4.4s format string with a pointer to >> the u32 is somewhat questionable, as well as dependent on byte >> order. There's a kernel extension format specifier %p4cc to format 4cc >> codes. Use it across the board in msm for pixel format logging. >> >> This should also fix the reported build warning: >> >> include/drm/drm_print.h:536:35: warning: '%4.4s' directive argument is >> null [-Wformat-overflow=] >> >> Reported-by: Aishwarya TCV >> Closes: >> https://lore.kernel.org/r/2ac758ce-a196-4e89-a397-488ba3101...@arm.com >> Signed-off-by: Jani Nikula >> >> --- >> >> Tip: 'git show --color-words -w' might be the easiest way to review. >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 +++ >> .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 +- >> drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 4 ++-- >> drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 24 +-- >> drivers/gpu/drm/msm/msm_fb.c | 10 >> 5 files changed, 24 insertions(+), 24 deletions(-) > > Reviewed-by: Dmitry Baryshkov Thanks! Do you take this via the msm tree? BR, Jani. -- Jani Nikula, Intel
[PATCH] drm/msm: convert all pixel format logging to use %p4cc
Logging u32 pixel formats using %4.4s format string with a pointer to
the u32 is somewhat questionable, as well as dependent on byte
order. There's a kernel extension format specifier %p4cc to format 4cc
codes. Use it across the board in msm for pixel format logging.
This should also fix the reported build warning:
include/drm/drm_print.h:536:35: warning: '%4.4s' directive argument is
null [-Wformat-overflow=]
Reported-by: Aishwarya TCV
Closes: https://lore.kernel.org/r/2ac758ce-a196-4e89-a397-488ba3101...@arm.com
Signed-off-by: Jani Nikula
---
Tip: 'git show --color-words -w' might be the easiest way to review.
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 +++
.../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c | 4 ++--
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 24 +--
drivers/gpu/drm/msm/msm_fb.c | 10
5 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9a14d2232e4a..aa1e68379d9f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2203,8 +2203,8 @@ void dpu_encoder_helper_phys_setup_cdm(struct
dpu_encoder_phys *phys_enc,
return;
if (!DPU_FORMAT_IS_YUV(dpu_fmt)) {
- DPU_DEBUG("[enc:%d] cdm_disable fmt:%x\n",
DRMID(phys_enc->parent),
- dpu_fmt->base.pixel_format);
+ DPU_DEBUG("[enc:%d] cdm_disable fmt:%p4cc\n",
DRMID(phys_enc->parent),
+ _fmt->base.pixel_format);
if (hw_cdm->ops.bind_pingpong_blk)
hw_cdm->ops.bind_pingpong_blk(hw_cdm, PINGPONG_NONE);
@@ -2244,9 +2244,9 @@ void dpu_encoder_helper_phys_setup_cdm(struct
dpu_encoder_phys *phys_enc,
break;
}
- DPU_DEBUG("[enc:%d] cdm_enable:%d,%d,%X,%d,%d,%d,%d]\n",
+ DPU_DEBUG("[enc:%d] cdm_enable:%d,%d,%p4cc,%d,%d,%d,%d]\n",
DRMID(phys_enc->parent), cdm_cfg->output_width,
- cdm_cfg->output_height,
cdm_cfg->output_fmt->base.pixel_format,
+ cdm_cfg->output_height,
_cfg->output_fmt->base.pixel_format,
cdm_cfg->output_type, cdm_cfg->output_bit_depth,
cdm_cfg->h_cdwn_type, cdm_cfg->v_cdwn_type);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 1924a2b28e53..9dbb8ddcddec 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -580,7 +580,7 @@ static void dpu_encoder_phys_wb_prepare_wb_job(struct
dpu_encoder_phys *phys_enc
format->pixel_format, job->fb->modifier);
if (!wb_cfg->dest.format) {
/* this error should be detected during atomic_check */
- DPU_ERROR("failed to get format %x\n", format->pixel_format);
+ DPU_ERROR("failed to get format %p4cc\n",
>pixel_format);
return;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
index e366ab134249..95e6e58b1a21 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
@@ -647,8 +647,8 @@ static int _dpu_format_get_plane_sizes_ubwc(
color = _dpu_format_get_media_color_ubwc(fmt);
if (color < 0) {
- DRM_ERROR("UBWC format not supported for fmt: %4.4s\n",
- (char *)>base.pixel_format);
+ DRM_ERROR("UBWC format not supported for fmt: %p4cc\n",
+ >base.pixel_format);
return -EINVAL;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index ff975ad51145..ff4ac4daaeca 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -234,9 +234,9 @@ static int _dpu_plane_calc_fill_level(struct drm_plane
*plane,
}
}
- DPU_DEBUG_PLANE(pdpu, "pnum:%d fmt: %4.4s w:%u fl:%u\n",
+ DPU_DEBUG_PLANE(pdpu, "pnum:%d fmt: %p4cc w:%u fl:%u\n",
pipe->sspp->idx - SSPP_VIG0,
- (char *)>base.pixel_format,
+ >base.pixel_format,
src_width, total_fl);
return total_fl;
@@ -287,9 +287,9 @@ static void _dpu_plane_set_qos_lut(struct drm_plane *plane,
(fmt) ? fmt->base.pixel_format : 0,
pdpu->is_rt_pipe, total_fl, cfg.creq_lut, lut_usage);
-
Re: [PATCH v2] drm/dp: move intel_dp_vsc_sdp_pack() to generic helper
On Thu, 15 Feb 2024, Abhinav Kumar wrote:
> intel_dp_vsc_sdp_pack() can be re-used by other DRM drivers as well.
> Lets move this to drm_dp_helper to achieve this.
>
> changes in v2:
> - rebased on top of drm-tip
>
> Acked-by: Dmitry Baryshkov
> Signed-off-by: Abhinav Kumar
Acked-by: Jani Nikula
> ---
> drivers/gpu/drm/display/drm_dp_helper.c | 78 +
> drivers/gpu/drm/i915/display/intel_dp.c | 71 +-
> include/drm/display/drm_dp_helper.h | 3 +
> 3 files changed, 83 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
> b/drivers/gpu/drm/display/drm_dp_helper.c
> index 8d6ce46471ae..6c91f400ecb1 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -2913,6 +2913,84 @@ void drm_dp_vsc_sdp_log(struct drm_printer *p, const
> struct drm_dp_vsc_sdp *vsc)
> }
> EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
>
> +/**
> + * drm_dp_vsc_sdp_pack() - pack a given vsc sdp into generic dp_sdp
> + * @vsc: vsc sdp initialized according to its purpose as defined in
> + * table 2-118 - table 2-120 in DP 1.4a specification
> + * @sdp: valid handle to the generic dp_sdp which will be packed
> + * @size: valid size of the passed sdp handle
> + *
> + * Returns length of sdp on success and error code on failure
> + */
> +ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
> + struct dp_sdp *sdp, size_t size)
> +{
> + size_t length = sizeof(struct dp_sdp);
> +
> + if (size < length)
> + return -ENOSPC;
> +
> + memset(sdp, 0, size);
> +
> + /*
> + * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
> + * VSC SDP Header Bytes
> + */
> + sdp->sdp_header.HB0 = 0; /* Secondary-Data Packet ID = 0 */
> + sdp->sdp_header.HB1 = vsc->sdp_type; /* Secondary-data Packet Type */
> + sdp->sdp_header.HB2 = vsc->revision; /* Revision Number */
> + sdp->sdp_header.HB3 = vsc->length; /* Number of Valid Data Bytes */
> +
> + if (vsc->revision == 0x6) {
> + sdp->db[0] = 1;
> + sdp->db[3] = 1;
> + }
> +
> + /*
> + * Revision 0x5 and revision 0x7 supports Pixel Encoding/Colorimetry
> + * Format as per DP 1.4a spec and DP 2.0 respectively.
> + */
> + if (!(vsc->revision == 0x5 || vsc->revision == 0x7))
> + goto out;
> +
> + /* VSC SDP Payload for DB16 through DB18 */
> + /* Pixel Encoding and Colorimetry Formats */
> + sdp->db[16] = (vsc->pixelformat & 0xf) << 4; /* DB16[7:4] */
> + sdp->db[16] |= vsc->colorimetry & 0xf; /* DB16[3:0] */
> +
> + switch (vsc->bpc) {
> + case 6:
> + /* 6bpc: 0x0 */
> + break;
> + case 8:
> + sdp->db[17] = 0x1; /* DB17[3:0] */
> + break;
> + case 10:
> + sdp->db[17] = 0x2;
> + break;
> + case 12:
> + sdp->db[17] = 0x3;
> + break;
> + case 16:
> + sdp->db[17] = 0x4;
> + break;
> + default:
> + WARN(1, "Missing case %d\n", vsc->bpc);
> + return -EINVAL;
> + }
> +
> + /* Dynamic Range and Component Bit Depth */
> + if (vsc->dynamic_range == DP_DYNAMIC_RANGE_CTA)
> + sdp->db[17] |= 0x80; /* DB17[7] */
> +
> + /* Content Type */
> + sdp->db[18] = vsc->content_type & 0x7;
> +
> +out:
> + return length;
> +}
> +EXPORT_SYMBOL(drm_dp_vsc_sdp_pack);
> +
> /**
> * drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
> * @dpcd: DisplayPort configuration data
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 217196196e50..a9458df475e2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4089,73 +4089,6 @@ intel_dp_needs_vsc_sdp(const struct intel_crtc_state
> *crtc_state,
> return false;
> }
>
> -static ssize_t intel_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc,
> - struct dp_sdp *sdp, size_t size)
> -{
> - size_t length = sizeof(struct dp_sdp);
> -
> - if (size < length)
> - return -ENOSPC;
> -
> - memset(sdp, 0, size);
> -
> - /*
> - * Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
> - * VSC SDP Header Bytes
> - */
> - sdp->sdp_header.HB0 = 0; /* Secondary-Data
Re: [PATCH v3] drm/dp: add an API to indicate if sink supports VSC SDP
On Sat, 10 Feb 2024, Abhinav Kumar wrote:
> From: Paloma Arellano
>
> YUV420 format is supported only in the VSC SDP packet and not through
> MSA. Hence add an API which indicates the sink support which can be used
> by the rest of the DP programming.
>
> changes in v3:
> - fix the commit title prefix to drm/dp
> - get rid of redundant !!
> - break out this change from series [1] to get acks from drm core
> maintainers
>
> Changes in v2:
> - Move VSC SDP support check API from dp_panel.c to
> drm_dp_helper.c
>
> [1]: https://patchwork.freedesktop.org/series/129180/
>
> Reviewed-by: Dmitry Baryshkov
> Signed-off-by: Paloma Arellano
> Signed-off-by: Abhinav Kumar
> ---
> drivers/gpu/drm/display/drm_dp_helper.c | 21 +
> include/drm/display/drm_dp_helper.h | 1 +
> 2 files changed, 22 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
> b/drivers/gpu/drm/display/drm_dp_helper.c
> index b1ca3a1100da..7a851f92b249 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -2916,6 +2916,27 @@ void drm_dp_vsc_sdp_log(const char *level, struct
> device *dev,
> }
> EXPORT_SYMBOL(drm_dp_vsc_sdp_log);
>
> +/**
> + * drm_dp_vsc_sdp_supported() - check if vsc sdp is supported
> + * @aux: DisplayPort AUX channel
> + * @dpcd: DisplayPort configuration data
> + *
> + * Returns true if vsc sdp is supported, else returns false
> + */
> +bool drm_dp_vsc_sdp_supported(struct drm_dp_aux *aux, const u8
> dpcd[DP_RECEIVER_CAP_SIZE])
> +{
> + u8 rx_feature;
> +
> + if (drm_dp_dpcd_readb(aux, DP_DPRX_FEATURE_ENUMERATION_LIST,
> _feature) != 1) {
> + drm_dbg_dp(aux->drm_dev, "failed to read
> DP_DPRX_FEATURE_ENUMERATION_LIST\n");
> + return false;
> + }
> +
> + return (dpcd[DP_DPCD_REV] >= DP_DPCD_REV_13) &&
> + (rx_feature & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED);
I guess you could bail out early without further dpcd access with the
dpcd rev check?
BR,
Jani.
> +}
> +EXPORT_SYMBOL(drm_dp_vsc_sdp_supported);
> +
> /**
> * drm_dp_get_pcon_max_frl_bw() - maximum frl supported by PCON
> * @dpcd: DisplayPort configuration data
> diff --git a/include/drm/display/drm_dp_helper.h
> b/include/drm/display/drm_dp_helper.h
> index 863b2e7add29..948381b2b0b1 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -100,6 +100,7 @@ struct drm_dp_vsc_sdp {
>
> void drm_dp_vsc_sdp_log(const char *level, struct device *dev,
> const struct drm_dp_vsc_sdp *vsc);
> +bool drm_dp_vsc_sdp_supported(struct drm_dp_aux *aux, const u8
> dpcd[DP_RECEIVER_CAP_SIZE]);
>
> int drm_dp_psr_setup_time(const u8 psr_cap[EDP_PSR_RECEIVER_CAP_SIZE]);
--
Jani Nikula, Intel
Re: [3/3] ASoC: hdmi-codec: drop drm/drm_edid.h include
On Fri, 12 Jan 2024, "Sarha, Jyri" wrote: > Reviewed-by: > > Thanks, > The including of drm_edid.h in hdmi-codec.h is a relic from my pre > upstreaming version of hdmi-codec. I don't think it was ever needed > in any upsteam version. Thanks for the reviews and acks, pushed to drm-misc-next, even if I didn't get an ack from Jaroslav or Takashi. Seems rather benign anyway. BR, Jani. -- Jani Nikula, Intel
Re: [PATCH 3/3] ASoC: hdmi-codec: drop drm/drm_edid.h include
On Thu, 04 Jan 2024, Jani Nikula wrote: > hdmi-codec.h does not appear to directly need drm/drm_edid.h for > anything. Remove it. Jaroslav, Takashi, ack for merging this via the drm trees, please? BR, Jani. > > There are some files that get drm/drm_edid.h by proxy; include it where > needed. > > v2-v4: Fix build (kernel test robot ) > > Cc: Rob Clark > Cc: Abhinav Kumar > Cc: Dmitry Baryshkov > Cc: Sean Paul > Cc: Marijn Suijten > Cc: linux-arm-...@vger.kernel.org > Cc: freedreno@lists.freedesktop.org > Cc: Andrzej Hajda > Cc: Neil Armstrong > Cc: Robert Foss > Cc: Laurent Pinchart > Cc: Jonas Karlman > Cc: Jernej Skrabec > Cc: Jaroslav Kysela > Cc: Takashi Iwai > Cc: linux-so...@vger.kernel.org > Acked-by: Maxime Ripard > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/bridge/lontium-lt9611.c| 1 + > drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 1 + > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 1 + > drivers/gpu/drm/msm/dp/dp_display.c| 1 + > drivers/gpu/drm/tegra/hdmi.c | 1 + > drivers/gpu/drm/vc4/vc4_hdmi.c | 1 + > include/sound/hdmi-codec.h | 1 - > 7 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c > b/drivers/gpu/drm/bridge/lontium-lt9611.c > index 9663601ce098..b9205d14d943 100644 > --- a/drivers/gpu/drm/bridge/lontium-lt9611.c > +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c > @@ -18,6 +18,7 @@ > > #include > #include > +#include > #include > #include > #include > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c > b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c > index e971b75e90ad..f3f130c1ef0a 100644 > --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c > +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c > @@ -21,6 +21,7 @@ > > #include > #include > +#include > #include > #include > #include > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index 52d91a0df85e..fa63a21bdd1c 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -31,6 +31,7 @@ > #include > #include > #include > +#include > #include > #include > #include > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index d37d599aec27..c8e1bbebdffe 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -11,6 +11,7 @@ > #include > #include > #include > +#include > > #include "msm_drv.h" > #include "msm_kms.h" > diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c > index 417fb884240a..09987e372e3e 100644 > --- a/drivers/gpu/drm/tegra/hdmi.c > +++ b/drivers/gpu/drm/tegra/hdmi.c > @@ -24,6 +24,7 @@ > #include > #include > #include > +#include > #include > #include > #include > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index f05e2c95a60d..34f807ed1c31 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -35,6 +35,7 @@ > #include > #include > #include > +#include > #include > #include > #include > diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h > index 9b162ac1e08e..5e1a9eafd10f 100644 > --- a/include/sound/hdmi-codec.h > +++ b/include/sound/hdmi-codec.h > @@ -12,7 +12,6 @@ > > #include > #include > -#include > #include > #include > #include -- Jani Nikula, Intel
[PATCH 3/3] ASoC: hdmi-codec: drop drm/drm_edid.h include
hdmi-codec.h does not appear to directly need drm/drm_edid.h for anything. Remove it. There are some files that get drm/drm_edid.h by proxy; include it where needed. v2-v4: Fix build (kernel test robot ) Cc: Rob Clark Cc: Abhinav Kumar Cc: Dmitry Baryshkov Cc: Sean Paul Cc: Marijn Suijten Cc: linux-arm-...@vger.kernel.org Cc: freedreno@lists.freedesktop.org Cc: Andrzej Hajda Cc: Neil Armstrong Cc: Robert Foss Cc: Laurent Pinchart Cc: Jonas Karlman Cc: Jernej Skrabec Cc: Jaroslav Kysela Cc: Takashi Iwai Cc: linux-so...@vger.kernel.org Acked-by: Maxime Ripard Signed-off-by: Jani Nikula --- drivers/gpu/drm/bridge/lontium-lt9611.c| 1 + drivers/gpu/drm/bridge/lontium-lt9611uxc.c | 1 + drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 1 + drivers/gpu/drm/msm/dp/dp_display.c| 1 + drivers/gpu/drm/tegra/hdmi.c | 1 + drivers/gpu/drm/vc4/vc4_hdmi.c | 1 + include/sound/hdmi-codec.h | 1 - 7 files changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c index 9663601ce098..b9205d14d943 100644 --- a/drivers/gpu/drm/bridge/lontium-lt9611.c +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c @@ -18,6 +18,7 @@ #include #include +#include #include #include #include diff --git a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c index e971b75e90ad..f3f130c1ef0a 100644 --- a/drivers/gpu/drm/bridge/lontium-lt9611uxc.c +++ b/drivers/gpu/drm/bridge/lontium-lt9611uxc.c @@ -21,6 +21,7 @@ #include #include +#include #include #include #include diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 52d91a0df85e..fa63a21bdd1c 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index d37d599aec27..c8e1bbebdffe 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -11,6 +11,7 @@ #include #include #include +#include #include "msm_drv.h" #include "msm_kms.h" diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c index 417fb884240a..09987e372e3e 100644 --- a/drivers/gpu/drm/tegra/hdmi.c +++ b/drivers/gpu/drm/tegra/hdmi.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index f05e2c95a60d..34f807ed1c31 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -35,6 +35,7 @@ #include #include #include +#include #include #include #include diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index 9b162ac1e08e..5e1a9eafd10f 100644 --- a/include/sound/hdmi-codec.h +++ b/include/sound/hdmi-codec.h @@ -12,7 +12,6 @@ #include #include -#include #include #include #include -- 2.39.2
Re: [Freedreno] [Intel-gfx] [PATCH v4 00/20] remove I2C_CLASS_DDC support
On Mon, 20 Nov 2023, Heiner Kallweit wrote: > v4: > - more ack and review tags Please do not send new versions just to record the acks and reviews. They should be added while applying the patches. Thanks, Jani. -- Jani Nikula, Intel
Re: [Freedreno] [PATCH] drm/msm/dp: skip validity check for DP CTS EDID checksum
On Tue, 12 Sep 2023, Abhinav Kumar wrote:
> Hi Jani
>
> On 9/12/2023 5:16 AM, Jani Nikula wrote:
>> On Thu, 07 Sep 2023, Stephen Boyd wrote:
>>> Quoting Jani Nikula (2023-09-01 07:20:34)
>>>> The DP CTS test for EDID last block checksum expects the checksum for
>>>> the last block, invalid or not. Skip the validity check.
>>>>
>>>> For the most part (*), the EDIDs returned by drm_get_edid() will be
>>>> valid anyway, and there's the CTS workaround to get the checksum for
>>>> completely invalid EDIDs. See commit 7948fe12d47a ("drm/msm/dp: return
>>>> correct edid checksum after corrupted edid checksum read").
>>>>
>>>> This lets us remove one user of drm_edid_block_valid() with hopes the
>>>> function can be removed altogether in the future.
>>>>
>>>> (*) drm_get_edid() ignores checksum errors on CTA extensions.
>>>>
>>>> Cc: Abhinav Kumar
>>>> Cc: Dmitry Baryshkov
>>>> Cc: Kuogee Hsieh
>>>> Cc: Marijn Suijten
>>>> Cc: Rob Clark
>>>> Cc: Sean Paul
>>>> Cc: Stephen Boyd
>>>> Cc: linux-arm-...@vger.kernel.org
>>>> Cc: freedreno@lists.freedesktop.org
>>>> Signed-off-by: Jani Nikula
>>>> ---
>>>
>>> Reviewed-by: Stephen Boyd
>>
>> Thanks; is that enough to merge? I can't claim I would have been able to
>> test this.
>>
>
> Reviewed-by: Abhinav Kumar
>
> Change looks fine.
>
> We can pick this up in the MSM tree if you would like.
I'd appreciate that, thanks!
BR,
Jani.
>
> Dmitry, you can please pick this up along with my R-b and Kuogee's R-b
> as well.
>
> I think his R-b got misformatted. I can ask him to add that again.
>
>>>
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> b/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> index 42d52510ffd4..86a8e06c7a60 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>>>> @@ -289,26 +289,9 @@ int dp_panel_get_modes(struct dp_panel *dp_panel,
>>>>
>>>> static u8 dp_panel_get_edid_checksum(struct edid *edid)
>>>
>>> It would be nice to make 'edid' const here in another patch.
>>
>> Sure.
>>
>> BR,
>> Jani.
>>
>>
--
Jani Nikula, Intel
Re: [Freedreno] [PATCH] drm/msm/dp: skip validity check for DP CTS EDID checksum
On Thu, 07 Sep 2023, Stephen Boyd wrote:
> Quoting Jani Nikula (2023-09-01 07:20:34)
>> The DP CTS test for EDID last block checksum expects the checksum for
>> the last block, invalid or not. Skip the validity check.
>>
>> For the most part (*), the EDIDs returned by drm_get_edid() will be
>> valid anyway, and there's the CTS workaround to get the checksum for
>> completely invalid EDIDs. See commit 7948fe12d47a ("drm/msm/dp: return
>> correct edid checksum after corrupted edid checksum read").
>>
>> This lets us remove one user of drm_edid_block_valid() with hopes the
>> function can be removed altogether in the future.
>>
>> (*) drm_get_edid() ignores checksum errors on CTA extensions.
>>
>> Cc: Abhinav Kumar
>> Cc: Dmitry Baryshkov
>> Cc: Kuogee Hsieh
>> Cc: Marijn Suijten
>> Cc: Rob Clark
>> Cc: Sean Paul
>> Cc: Stephen Boyd
>> Cc: linux-arm-...@vger.kernel.org
>> Cc: freedreno@lists.freedesktop.org
>> Signed-off-by: Jani Nikula
>> ---
>
> Reviewed-by: Stephen Boyd
Thanks; is that enough to merge? I can't claim I would have been able to
test this.
>
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
>> b/drivers/gpu/drm/msm/dp/dp_panel.c
>> index 42d52510ffd4..86a8e06c7a60 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>> @@ -289,26 +289,9 @@ int dp_panel_get_modes(struct dp_panel *dp_panel,
>>
>> static u8 dp_panel_get_edid_checksum(struct edid *edid)
>
> It would be nice to make 'edid' const here in another patch.
Sure.
BR,
Jani.
--
Jani Nikula, Intel
[Freedreno] [PATCH] drm/msm/dp: skip validity check for DP CTS EDID checksum
The DP CTS test for EDID last block checksum expects the checksum for
the last block, invalid or not. Skip the validity check.
For the most part (*), the EDIDs returned by drm_get_edid() will be
valid anyway, and there's the CTS workaround to get the checksum for
completely invalid EDIDs. See commit 7948fe12d47a ("drm/msm/dp: return
correct edid checksum after corrupted edid checksum read").
This lets us remove one user of drm_edid_block_valid() with hopes the
function can be removed altogether in the future.
(*) drm_get_edid() ignores checksum errors on CTA extensions.
Cc: Abhinav Kumar
Cc: Dmitry Baryshkov
Cc: Kuogee Hsieh
Cc: Marijn Suijten
Cc: Rob Clark
Cc: Sean Paul
Cc: Stephen Boyd
Cc: linux-arm-...@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/msm/dp/dp_panel.c | 21 ++---
1 file changed, 2 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
b/drivers/gpu/drm/msm/dp/dp_panel.c
index 42d52510ffd4..86a8e06c7a60 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -289,26 +289,9 @@ int dp_panel_get_modes(struct dp_panel *dp_panel,
static u8 dp_panel_get_edid_checksum(struct edid *edid)
{
- struct edid *last_block;
- u8 *raw_edid;
- bool is_edid_corrupt = false;
+ edid += edid->extensions;
- if (!edid) {
- DRM_ERROR("invalid edid input\n");
- return 0;
- }
-
- raw_edid = (u8 *)edid;
- raw_edid += (edid->extensions * EDID_LENGTH);
- last_block = (struct edid *)raw_edid;
-
- /* block type extension */
- drm_edid_block_valid(raw_edid, 1, false, _edid_corrupt);
- if (!is_edid_corrupt)
- return last_block->checksum;
-
- DRM_ERROR("Invalid block, no checksum\n");
- return 0;
+ return edid->checksum;
}
void dp_panel_handle_sink_request(struct dp_panel *dp_panel)
--
2.39.2
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
> Hello Jani,
>
> On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:
>> On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
>> > Hello,
>> >
>> > while I debugged an issue in the imx-lcdc driver I was constantly
>> > irritated about struct drm_device pointer variables being named "dev"
>> > because with that name I usually expect a struct device pointer.
>> >
>> > I think there is a big benefit when these are all renamed to "drm_dev".
>> > I have no strong preference here though, so "drmdev" or "drm" are fine
>> > for me, too. Let the bikesheding begin!
>> >
>> > Some statistics:
>> >
>> > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq
>> > -c | sort -n
>> > 1 struct drm_device *adev_to_drm
>> > 1 struct drm_device *drm_
>> > 1 struct drm_device *drm_dev
>> > 1 struct drm_device*drm_dev
>> > 1 struct drm_device *pdev
>> > 1 struct drm_device *rdev
>> > 1 struct drm_device *vdev
>> > 2 struct drm_device *dcss_drv_dev_to_drm
>> > 2 struct drm_device **ddev
>> > 2 struct drm_device *drm_dev_alloc
>> > 2 struct drm_device *mock
>> > 2 struct drm_device *p_ddev
>> > 5 struct drm_device *device
>> > 9 struct drm_device * dev
>> > 25 struct drm_device *d
>> > 95 struct drm_device *
>> > 216 struct drm_device *ddev
>> > 234 struct drm_device *drm_dev
>> > 611 struct drm_device *drm
>> >4190 struct drm_device *dev
>> >
>> > This series starts with renaming struct drm_crtc::dev to drm_dev. If
>> > it's not only me and others like the result of this effort it should be
>> > followed up by adapting the other structs and the individual usages in
>> > the different drivers.
>>
>> I think this is an unnecessary change. In drm, a dev is usually a drm
>> device, i.e. struct drm_device *.
>
> Well, unless it's not. Prominently there is
>
> struct drm_device {
> ...
> struct device *dev;
> ...
> };
>
> which yields quite a few code locations using dev->dev which is
> IMHO unnecessary irritating:
>
> $ git grep '\dev' v6.5-rc1 drivers/gpu/drm | wc -l
> 1633
>
> Also the functions that deal with both a struct device and a struct
> drm_device often use "dev" for the struct device and then "ddev" for
> the drm_device (see for example amdgpu_device_get_pcie_replay_count()).
Why is specifically struct drm_device *dev so irritating to you?
You lead us to believe it's an outlier in kernel, something that goes
against common kernel style, but it's really not:
$ git grep -how "struct [A-Za-z0-9_]\+ \*dev" | sort | uniq -c | sort -rn |
head -20
38494 struct device *dev
16388 struct net_device *dev
4184 struct drm_device *dev
2780 struct pci_dev *dev
1916 struct comedi_device *dev
1510 struct mlx5_core_dev *dev
1057 struct mlx4_dev *dev
894 struct b43_wldev *dev
762 struct input_dev *dev
623 struct usbnet *dev
561 struct mlx5_ib_dev *dev
525 struct mt76_dev *dev
465 struct mt76x02_dev *dev
435 struct platform_device *dev
431 struct usb_device *dev
411 struct mt7915_dev *dev
398 struct cx231xx *dev
378 struct mei_device *dev
363 struct ksz_device *dev
359 struct mthca_dev *dev
A good portion of the above also have a dev member.
Are you planning on changing all of the above too, or are you only
annoyed by drm?
I'm really not convinced at all.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Wed, 12 Jul 2023, Uwe Kleine-König wrote: > Hello, > > while I debugged an issue in the imx-lcdc driver I was constantly > irritated about struct drm_device pointer variables being named "dev" > because with that name I usually expect a struct device pointer. > > I think there is a big benefit when these are all renamed to "drm_dev". > I have no strong preference here though, so "drmdev" or "drm" are fine > for me, too. Let the bikesheding begin! > > Some statistics: > > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c > | sort -n > 1 struct drm_device *adev_to_drm > 1 struct drm_device *drm_ > 1 struct drm_device *drm_dev > 1 struct drm_device*drm_dev > 1 struct drm_device *pdev > 1 struct drm_device *rdev > 1 struct drm_device *vdev > 2 struct drm_device *dcss_drv_dev_to_drm > 2 struct drm_device **ddev > 2 struct drm_device *drm_dev_alloc > 2 struct drm_device *mock > 2 struct drm_device *p_ddev > 5 struct drm_device *device > 9 struct drm_device * dev > 25 struct drm_device *d > 95 struct drm_device * > 216 struct drm_device *ddev > 234 struct drm_device *drm_dev > 611 struct drm_device *drm >4190 struct drm_device *dev > > This series starts with renaming struct drm_crtc::dev to drm_dev. If > it's not only me and others like the result of this effort it should be > followed up by adapting the other structs and the individual usages in > the different drivers. I think this is an unnecessary change. In drm, a dev is usually a drm device, i.e. struct drm_device *. As shown by the numbers above. If folks insist on following through with this anyway, I'm firmly in the camp the name should be "drm" and nothing else. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH v12 1/9] drm/display/dsc: Add flatness and initial scale value calculations
On Wed, 17 May 2023, Jessica Zhang wrote:
> Add helpers to calculate det_thresh_flatness and initial_scale_value as
> these calculations are defined within the DSC spec.
>
> Reviewed-by: Marijn Suijten
> Signed-off-by: Jessica Zhang
> ---
> include/drm/display/drm_dsc_helper.h | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/include/drm/display/drm_dsc_helper.h
> b/include/drm/display/drm_dsc_helper.h
> index fc2104415dcb..753b0034eda7 100644
> --- a/include/drm/display/drm_dsc_helper.h
> +++ b/include/drm/display/drm_dsc_helper.h
> @@ -25,5 +25,15 @@ void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config
> *vdsc_cfg);
> int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum
> drm_dsc_params_type type);
> int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
>
> +static inline u8 drm_dsc_initial_scale_value(const struct drm_dsc_config
> *dsc)
> +{
> + return 8 * dsc->rc_model_size / (dsc->rc_model_size -
> dsc->initial_offset);
> +}
> +
> +static inline u32 drm_dsc_flatness_det_thresh(const struct drm_dsc_config
> *dsc)
> +{
> + return 2 << (dsc->bits_per_component - 8);
> +}
kernel-doc? Maybe make them regular functions instead of static inline?
BR,
Jani.
> +
> #endif /* _DRM_DSC_HELPER_H_ */
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH v12 3/9] drm/display/dsc: Add drm_dsc_get_bpp_int helper
On Wed, 17 May 2023, Jessica Zhang wrote:
> Add helper to get the integer value of drm_dsc_config.bits_per_pixel
>
> Reviewed-by: Marijn Suijten
> Signed-off-by: Jessica Zhang
> ---
> include/drm/display/drm_dsc_helper.h | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/include/drm/display/drm_dsc_helper.h
> b/include/drm/display/drm_dsc_helper.h
> index e0cbc38ada26..7bf7e8ff8eb4 100644
> --- a/include/drm/display/drm_dsc_helper.h
> +++ b/include/drm/display/drm_dsc_helper.h
> @@ -9,6 +9,7 @@
> #define DRM_DSC_HELPER_H_
>
> #include
> +#include
Seems like a small thing, adding static inlines in headers usually leads
to more header dependencies like this, and gradually slows down build.
Does it need to be a static inline?
BR,
Jani.
>
> enum drm_dsc_params_type {
> DRM_DSC_1_2_444,
> @@ -26,6 +27,12 @@ void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config
> *vdsc_cfg);
> int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum
> drm_dsc_params_type type);
> int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
>
> +static inline u32 drm_dsc_get_bpp_int(const struct drm_dsc_config *dsc)
> +{
> + WARN_ON_ONCE(dsc->bits_per_pixel & 0xf);
> + return dsc->bits_per_pixel >> 4;
> +}
> +
> static inline u8 drm_dsc_initial_scale_value(const struct drm_dsc_config
> *dsc)
> {
> return 8 * dsc->rc_model_size / (dsc->rc_model_size -
> dsc->initial_offset);
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH v5 0/8] drm/i915: move DSC RC tables to drm_dsc_helper.c
On Thu, 04 May 2023, Dmitry Baryshkov wrote: > Other platforms (msm) will benefit from sharing the DSC config setup > functions. This series moves parts of static DSC config data from the > i915 driver to the common helpers to be used by other drivers. > > Note: the RC parameters were cross-checked against config files found in > DSC model 2021062, 20161212 (and 20150914). The first patch modifies > tables according to those config files, while preserving parameter > values using the code. I have not changed one of the values in the > pre-SCR config file as it clearly looks like a typo in the config file, > considering the table E in DSC 1.1 and in the DSC 1.1 SCR. As I believe I've said before, I think it's fine to merge these either via drm-intel or drm-misc. Which do you prefer? BR, Jani. > > Chances since v4: > - Rebased on top of drm-intel-next > - Cut the first 8 patches of the series to ease merging. The rest of the > patches will go afterwards. > > Chances since v3: > - Rebased on top of drm-intel-next > - Dropped the msm patch to make patchset fully mergeable through > drm-intel > - Made drm_dsc_set_const_params() ignore rc_model_size, picked up > drm_dsc_set_initial_scale_value() patch by Jessica and switched > intel_vdsc.c to use those two helpers. > - Added a patch to make i915 actually use rc_tgt_offset_high, > rc_tgt_offset_low and rc_edge_factor from struct drm_dsc_config. > > Chances since v2: > - Rebased on top of drm-intel-next > > Chances since v1: > - Made drm_dsc_rc_buf_thresh static rather than exporting it > - Switched drm_dsc_rc_buf_thresh loop to use ARRAY_SIZE. Added > BUILD_BUG_ON's to be sure that array sizes are correct > - Fixed rc_parameters_data indentation to be logical and tidy > - Fixed drm_dsc_setup_rc_params() kerneldoc > - Added a clause to drm_dsc_setup_rc_params() to verify bpp and bpc > being set. > - Fixed range_bpg_offset programming in calculate_rc_params() > - Fixed bpp vs bpc bug in intel_dsc_compute_params() > - Added FIXME comment next to the customizations in > intel_dsc_compute_params(). > > Dmitry Baryshkov (8): > drm/i915/dsc: change DSC param tables to follow the DSC model > drm/i915/dsc: move rc_buf_thresh values to common helper > drm/i915/dsc: move DSC tables to DRM DSC helper > drm/i915/dsc: stop using interim structure for calculated params > drm/display/dsc: use flat array for rc_parameters lookup > drm/display/dsc: split DSC 1.2 and DSC 1.1 (pre-SCR) parameters > drm/display/dsc: include the rest of pre-SCR parameters > drm/display/dsc: add YCbCr 4:2:2 and 4:2:0 RC parameters > > drivers/gpu/drm/display/drm_dsc_helper.c | 986 ++ > drivers/gpu/drm/i915/display/intel_vdsc.c | 443 ++ > include/drm/display/drm_dsc_helper.h | 9 + > 3 files changed, 1042 insertions(+), 396 deletions(-) -- Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH v9 01/10] drm/hdcp: Add drm_hdcp_atomic_check()
On Tue, 18 Apr 2023, "Kandpal, Suraj" wrote:
> Yacoub
>> ; linux-ker...@vger.kernel.org
>> Subject: [PATCH v9 01/10] drm/hdcp: Add drm_hdcp_atomic_check()
>>
>> From: Sean Paul
>>
>> Move the hdcp atomic check from i915 to drm_hdcp so other drivers can use
>> it. No functional changes, just cleaned up some of the code when moving it
>> over.
>>
>> Acked-by: Jani Nikula
>> Reviewed-by: Rodrigo Vivi
>> Reviewed-by: Dmitry Baryshkov
>> Signed-off-by: Sean Paul
>> Signed-off-by: Mark Yacoub
>>
>> ---
>> Changes in v2:
>> -None
>> Changes in v3:
>> -None
>> Changes in v4:
>> -None
>> Changes in v5:
>> -None
>> Changes in v6:
>> -Rebase: move helper from drm_hdcp.c to drm_hdcp_helper.c Changes in
>> v7:
>> -Removed links to patch from commit msg (Dmitry Baryshkov)
>>
>> drivers/gpu/drm/display/drm_hdcp_helper.c | 64
>> +
>> drivers/gpu/drm/i915/display/intel_atomic.c | 4 +-
>> drivers/gpu/drm/i915/display/intel_hdcp.c | 47 ---
>> drivers/gpu/drm/i915/display/intel_hdcp.h | 3 -
>> include/drm/display/drm_hdcp_helper.h | 3 +
>> 5 files changed, 69 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/display/drm_hdcp_helper.c
>> b/drivers/gpu/drm/display/drm_hdcp_helper.c
>> index e78999c72bd77..7ca390b3ea106 100644
>> --- a/drivers/gpu/drm/display/drm_hdcp_helper.c
>> +++ b/drivers/gpu/drm/display/drm_hdcp_helper.c
>> @@ -20,6 +20,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> static inline void drm_hdcp_print_ksv(const u8 *ksv) { @@ -419,3 +420,66
>> @@ void drm_hdcp_update_content_protection(struct drm_connector
>> *connector,
>>dev-
>> >mode_config.content_protection_property);
>> }
>> EXPORT_SYMBOL(drm_hdcp_update_content_protection);
>> +
>> +/**
>> + * drm_hdcp_atomic_check - Helper for drivers to call during
>> +connector->atomic_check
>> + *
>> + * @state: pointer to the atomic state being checked
>> + * @connector: drm_connector on which content protection state needs an
>> +update
>> + *
>> + * This function can be used by display drivers to perform an atomic
>> +check on the
>> + * hdcp state elements. If hdcp state has changed, this function will
>> +set
>> + * mode_changed on the crtc driving the connector so it can update its
>> +hardware
>> + * to match the hdcp state.
>> + */
>> +void drm_hdcp_atomic_check(struct drm_connector *connector,
>> +struct drm_atomic_state *state)
>> +{
>> + struct drm_connector_state *new_conn_state, *old_conn_state;
>> + struct drm_crtc_state *new_crtc_state;
>> + u64 old_hdcp, new_hdcp;
>> +
>> + old_conn_state = drm_atomic_get_old_connector_state(state,
>> connector);
>> + old_hdcp = old_conn_state->content_protection;
>> +
>> + new_conn_state = drm_atomic_get_new_connector_state(state,
>> connector);
>> + new_hdcp = new_conn_state->content_protection;
>> +
>
> Nit: Why not assign these as right when they are declared we would end up
> using less lines
The rule of thumb is to only initialize trivial stuff at declaration.
BR,
Jani.
>
>> + if (!new_conn_state->crtc) {
>> + /*
>> + * If the connector is being disabled with CP enabled, mark it
>> + * desired so it's re-enabled when the connector is brought
>> back
>> + */
>> + if (old_hdcp ==
>> DRM_MODE_CONTENT_PROTECTION_ENABLED)
>> + new_conn_state->content_protection =
>> +
>> DRM_MODE_CONTENT_PROTECTION_DESIRED;
>> + return;
>> + }
>> +
>> + new_crtc_state =
>> + drm_atomic_get_new_crtc_state(state, new_conn_state-
>> >crtc);
>> + if (drm_atomic_crtc_needs_modeset(new_crtc_state) &&
>> + (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
>> + new_hdcp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED))
>> + new_conn_state->content_protection =
>> + DRM_MODE_CONTENT_PROTECTION_DESIRED;
>> +
>> + /*
>> + * Nothing to do if content type is unchanged and one of:
>> + * - state didn't change
>> + * - HDCP was activated since the last commit
>> + * - attempting to set to desired while already enabled
>&
Re: [Freedreno] [PATCH v2 06/10] drm/display/dsc: split DSC 1.2 and DSC 1.1 (pre-SCR) parameters
On Wed, 08 Mar 2023, Dmitry Baryshkov wrote:
> On Wed, 8 Mar 2023 at 12:14, Jani Nikula wrote:
>>
>> On Tue, 07 Mar 2023, Dmitry Baryshkov wrote:
>> > The array of rc_parameters contains a mixture of parameters from DSC 1.1
>> > and DSC 1.2 standards. Split these tow configuration arrays in
>> > preparation to adding more configuration data.
>> >
>> > Signed-off-by: Dmitry Baryshkov
>> > ---
>> > drivers/gpu/drm/display/drm_dsc_helper.c | 127 ++
>> > drivers/gpu/drm/i915/display/intel_vdsc.c | 10 +-
>> > include/drm/display/drm_dsc_helper.h | 7 +-
>> > 3 files changed, 119 insertions(+), 25 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c
>> > b/drivers/gpu/drm/display/drm_dsc_helper.c
>> > index acb93d4116e0..35b39f3109c4 100644
>> > --- a/drivers/gpu/drm/display/drm_dsc_helper.c
>> > +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
>> > @@ -324,11 +324,81 @@ struct rc_parameters_data {
>> >
>> > #define DSC_BPP(bpp) ((bpp) << 4)
>> >
>> > +static const struct rc_parameters_data rc_parameters_pre_scr[] = {
>> > + {
>> > + .bpp = DSC_BPP(8), .bpc = 8,
>> > + { 512, 12, 6144, 3, 12, 11, 11, {
>> > + { 0, 4, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
>> > + { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8
>> > },
>> > + { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 }, { 5,
>> > 12, -12 },
>> > + { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
>> > + }
>> > + }
>> > + },
>> > + {
>> > + .bpp = DSC_BPP(8), .bpc = 10,
>> > + { 512, 12, 6144, 7, 16, 15, 15, {
>> > + /*
>> > + * DSC model/pre-SCR-cfg has 8 for range_max_qp[0],
>> > however
>> > + * VESA DSC 1.1 Table E-5 sets it to 4.
>> > + */
>> > + { 0, 4, 2 }, { 4, 8, 0 }, { 5, 9, 0 }, { 5, 10, -2 },
>> > + { 7, 11, -4 }, { 7, 11, -6 }, { 7, 11, -8 }, { 7,
>> > 12, -8 },
>> > + { 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9,
>> > 16, -12 },
>> > + { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
>> > + }
>> > + }
>> > + },
>> > + {
>> > + .bpp = DSC_BPP(8), .bpc = 12,
>> > + { 512, 12, 6144, 11, 20, 19, 19, {
>> > + { 0, 12, 2 }, { 4, 12, 0 }, { 9, 13, 0 }, { 9, 14,
>> > -2 },
>> > + { 11, 15, -4 }, { 11, 15, -6 }, { 11, 15, -8 }, {
>> > 11, 16, -8 },
>> > + { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
>> > + { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
>> > + { 21, 23, -12 }
>> > + }
>> > + }
>> > + },
>> > + {
>> > + .bpp = DSC_BPP(12), .bpc = 8,
>> > + { 341, 15, 2048, 3, 12, 11, 11, {
>> > + { 0, 2, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
>> > + { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8
>> > },
>> > + { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 },
>> > + { 5, 12, -12 }, { 5, 13, -12 }, { 7, 13, -12 }, {
>> > 13, 15, -12 }
>> > + }
>> > + }
>> > + },
>> > + {
>> > + .bpp = DSC_BPP(12), .bpc = 10,
>> > + { 341, 15, 2048, 7, 16, 15, 15, {
>> > + { 0, 2, 2 }, { 2, 5, 0 }, { 3, 7, 0 }, { 4, 8, -2 },
>> > + { 6, 9, -4 }, { 7, 10, -6 }, { 7, 11, -8 }, { 7, 12,
>> > -8 },
>> > + { 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9,
>> > 16, -12 },
>> > + { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
>> > + }
>> > + }
>> > + },
>> > + {
>> > + .bpp = DSC_BPP(12), .bpc = 12,
>> > + { 341, 15, 2048, 11, 20, 19, 19, {
>> > + { 0, 6, 2 }, { 4, 9, 0 }, { 7, 11, 0 }, { 8, 12, -2
&
Re: [Freedreno] [PATCH v2 06/10] drm/display/dsc: split DSC 1.2 and DSC 1.1 (pre-SCR) parameters
0 }, { 13, 19, -10 },
> - { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> - { 21, 23, -12 }
> + { 11, 16, -8 }, { 11, 17, -10 }, { 13, 17, -10 },
> + { 13, 17, -12 }, { 13, 17, -12 }, { 15, 18, -12 },
> + { 18, 19, -12 }
> }
> }
> },
> @@ -602,7 +668,8 @@ static const struct rc_parameters_data rc_parameters[] = {
> { /* sentinel */ }
> };
>
> -static const struct rc_parameters *get_rc_params(u16 dsc_bpp,
> +static const struct rc_parameters *get_rc_params(const struct
> rc_parameters_data *rc_parameters,
> + u16 dsc_bpp,
>u8 bits_per_component)
> {
> int i;
> @@ -622,11 +689,13 @@ static const struct rc_parameters *get_rc_params(u16
> dsc_bpp,
> * function.
> *
> * @vdsc_cfg: DSC Configuration data partially filled by driver
> + * @kind: operating mode and standard to follow
> *
> * Return: 0 or -error code in case of an error
> */
> -int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg)
> +int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum
> drm_dsc_params_kind kind)
> {
> + const struct rc_parameters_data *data;
> const struct rc_parameters *rc_params;
> int i;
>
> @@ -634,7 +703,19 @@ int drm_dsc_setup_rc_params(struct drm_dsc_config
> *vdsc_cfg)
>!vdsc_cfg->bits_per_component))
> return -EINVAL;
>
> - rc_params = get_rc_params(vdsc_cfg->bits_per_pixel,
> + switch (kind) {
> + case DRM_DSC_1_2_444:
> + data = rc_parameters_1_2_444;
> + break;
> + case DRM_DSC_1_1_PRE_SCR:
> + data = rc_parameters_pre_scr;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + rc_params = get_rc_params(data,
> + vdsc_cfg->bits_per_pixel,
> vdsc_cfg->bits_per_component);
> if (!rc_params)
> return -EINVAL;
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index 20a4c2f343fe..a4d1d2ba71bb 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -157,7 +157,15 @@ int intel_dsc_compute_params(struct intel_crtc_state
> *pipe_config)
> if (DISPLAY_VER(dev_priv) >= 13) {
> calculate_rc_params(vdsc_cfg);
> } else {
> - ret = drm_dsc_setup_rc_params(vdsc_cfg);
> + if ((compressed_bpp == 8 ||
> + compressed_bpp == 12) &&
> + (vdsc_cfg->bits_per_component == 8 ||
> + vdsc_cfg->bits_per_component == 10 ||
> + vdsc_cfg->bits_per_component == 12))
> + ret = drm_dsc_setup_rc_params(vdsc_cfg,
> DRM_DSC_1_1_PRE_SCR);
> + else
> + ret = drm_dsc_setup_rc_params(vdsc_cfg,
> DRM_DSC_1_2_444);
> +
> if (ret)
> return ret;
>
> diff --git a/include/drm/display/drm_dsc_helper.h
> b/include/drm/display/drm_dsc_helper.h
> index 1681791f65a5..c634bb2935d3 100644
> --- a/include/drm/display/drm_dsc_helper.h
> +++ b/include/drm/display/drm_dsc_helper.h
> @@ -10,12 +10,17 @@
>
> #include
>
> +enum drm_dsc_params_kind {
> + DRM_DSC_1_2_444,
> + DRM_DSC_1_1_PRE_SCR, /* legacy params from DSC 1.1 */
> +};
> +
> void drm_dsc_dp_pps_header_init(struct dp_sdp_header *pps_header);
> int drm_dsc_dp_rc_buffer_size(u8 rc_buffer_block_size, u8 rc_buffer_size);
> void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_sdp,
> const struct drm_dsc_config *dsc_cfg);
> void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg);
> -int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg);
> +int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum
> drm_dsc_params_kind kind);
> int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
>
> #endif /* _DRM_DSC_HELPER_H_ */
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH 06/10] drm/display/dsc: split DSC 1.2 and DSC 1.1 (pre-SCR) parameters
> vdsc_cfg->bits_per_component);
> if (!rc_params)
> return -EINVAL;
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index 1ee8d13c9d64..4d220d24fa73 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -156,7 +156,15 @@ int intel_dsc_compute_params(struct intel_crtc_state
> *pipe_config)
> if (DISPLAY_VER(dev_priv) >= 13) {
> calculate_rc_params(vdsc_cfg);
> } else {
> - ret = drm_dsc_setup_rc_params(vdsc_cfg);
> + if ((compressed_bpp == 8 ||
> + compressed_bpp == 12) &&
> + (vdsc_cfg->bits_per_pixel == 8 ||
> + vdsc_cfg->bits_per_pixel == 10 ||
> + vdsc_cfg->bits_per_pixel == 12))
> + ret = drm_dsc_setup_rc_params(vdsc_cfg,
> DRM_DSC_1_1_PRE_SCR);
> + else
> + ret = drm_dsc_setup_rc_params(vdsc_cfg,
> DRM_DSC_1_2_444);
> +
> if (ret)
> return ret;
>
> diff --git a/include/drm/display/drm_dsc_helper.h
> b/include/drm/display/drm_dsc_helper.h
> index 1681791f65a5..c634bb2935d3 100644
> --- a/include/drm/display/drm_dsc_helper.h
> +++ b/include/drm/display/drm_dsc_helper.h
> @@ -10,12 +10,17 @@
>
> #include
>
> +enum drm_dsc_params_kind {
> + DRM_DSC_1_2_444,
> + DRM_DSC_1_1_PRE_SCR, /* legacy params from DSC 1.1 */
> +};
> +
> void drm_dsc_dp_pps_header_init(struct dp_sdp_header *pps_header);
> int drm_dsc_dp_rc_buffer_size(u8 rc_buffer_block_size, u8 rc_buffer_size);
> void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_sdp,
> const struct drm_dsc_config *dsc_cfg);
> void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg);
> -int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg);
> +int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum
> drm_dsc_params_kind kind);
> int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
>
> #endif /* _DRM_DSC_HELPER_H_ */
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH 07/10] drm/display/dsc: include the rest of pre-SCR parameters
On Tue, 28 Feb 2023, Dmitry Baryshkov wrote:
> DSC model contains pre-SCR RC parameters for other bpp/bpc combinations,
> include them here for completeness.
Need to run now, note to self:
Does i915 use the arrays to limit the bpp/bpc combos supported by
hardware? Do we need to add separate limiting in i915.
BR,
Jani.
>
> Signed-off-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/display/drm_dsc_helper.c | 72
> 1 file changed, 72 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c
> b/drivers/gpu/drm/display/drm_dsc_helper.c
> index 51794b40526a..1612536014ea 100644
> --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> @@ -327,6 +327,16 @@ struct rc_parameters_data {
> #define DSC_BPP(bpp) ((bpp) << 4)
>
> static const struct rc_parameters_data rc_parameters_pre_scr[] = {
> +{ DSC_BPP(6), 8,
> + /* 6BPP/8BPC */
> + { 683, 15, 6144, 3, 13, 11, 11, {
> + { 0, 2, 0 }, { 1, 4, -2 }, { 3, 6, -2 }, { 4, 6, -4 },
> + { 5, 7, -6 }, { 5, 7, -6 }, { 6, 7, -6 }, { 6, 8, -8 },
> + { 7, 9, -8 }, { 8, 10, -10 }, { 9, 11, -10 }, { 10, 12, -12 },
> + { 10, 13, -12 }, { 12, 14, -12 }, { 15, 15, -12 }
> + }
> + }
> +},
> { DSC_BPP(8), 8,
> /* 8BPP/8BPC */
> { 512, 12, 6144, 3, 12, 11, 11, {
> @@ -362,6 +372,37 @@ static const struct rc_parameters_data
> rc_parameters_pre_scr[] = {
> }
> }
> },
> +{ DSC_BPP(10), 8,
> + /* 10BPP/8BPC */
> + { 410, 12, 5632, 3, 12, 11, 11, {
> + { 0, 3, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 2, 6, -2 },
> + { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> + { 3, 9, -8 }, { 3, 9, -10 }, { 5, 10, -10 }, { 5, 11, -10 },
> + { 5, 12, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> + }
> + }
> +},
> +{ DSC_BPP(10), 10,
> + /* 10BPP/10BPC */
> + { 410, 12, 5632, 7, 16, 15, 15, {
> + { 0, 7, 2 }, { 4, 8, 0 }, { 5, 9, 0 }, { 6, 10, -2 },
> + { 7, 11, -4 }, { 7, 11, -6 }, { 7, 11, -8 }, { 7, 12, -8 },
> + { 7, 13, -8 }, { 7, 13, -10 }, { 9, 14, -10 }, { 9, 15, -10 },
> + { 9, 16, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> + }
> + }
> +},
> +{ DSC_BPP(10), 12,
> + /* 10BPP/12BPC */
> + { 410, 12, 5632, 11, 20, 19, 19, {
> + { 0, 11, 2 }, { 4, 12, 0 }, { 9, 13, 0 }, { 10, 14, -2 },
> + { 11, 15, -4 }, { 11, 15, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> + { 11, 17, -8 }, { 11, 17, -10 }, { 13, 18, -10 },
> + { 13, 19, -10 }, { 13, 20, -12 }, { 15, 21, -12 },
> + { 21, 23, -12 }
> + }
> + }
> +},
> { DSC_BPP(12), 8,
> /* 12BPP/8BPC */
> { 341, 15, 2048, 3, 12, 11, 11, {
> @@ -393,6 +434,37 @@ static const struct rc_parameters_data
> rc_parameters_pre_scr[] = {
> }
> }
> },
> +{ DSC_BPP(15), 8,
> + /* 15BPP/8BPC */
> + { 273, 15, 2048, 3, 12, 11, 11, {
> + { 0, 0, 10 }, { 0, 1, 8 }, { 0, 1, 6 }, { 0, 2, 4 },
> + { 1, 2, 2 }, { 1, 3, 0 }, { 1, 4, -2 }, { 2, 4, -4 },
> + { 3, 4, -6 }, { 3, 5, -8 }, { 4, 6, -10 }, { 5, 7, -10 },
> + { 5, 8, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> + }
> + }
> +},
> +{ DSC_BPP(15), 10,
> + /* 15BPP/10BPC */
> + { 273, 15, 2048, 7, 16, 15, 15, {
> + { 0, 2, 10 }, { 2, 5, 8 }, { 3, 5, 6 }, { 4, 6, 4 },
> + { 5, 6, 2 }, { 5, 7, 0 }, { 5, 8, -2 }, { 6, 8, -4 },
> + { 7, 8, -6 }, { 7, 9, -8 }, { 8, 10, -10 }, { 9, 11, -10 },
> + { 9, 12, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> + }
> + }
> +},
> +{ DSC_BPP(15), 12,
> + /* 15BPP/12BPC */
> + { 273, 15, 2048, 11, 20, 19, 19, {
> + { 0, 4, 10 }, { 2, 7, 8 }, { 4, 9, 6 }, { 6, 11, 4 },
> + { 9, 11, 2 }, { 9, 11, 0 }, { 9, 12, -2 }, { 10, 12, -4 },
> + { 11, 12, -6 }, { 11, 13, -8 }, { 12, 14, -10 },
> + { 13, 15, -10 }, { 13, 16, -12 }, { 15, 21, -12 },
> + { 21, 23, -12 }
> + }
> + }
> +},
> { /* sentinel */ }
> };
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH 05/10] drm/display/dsc: use flat array for rc_parameters lookup
On Tue, 28 Feb 2023, Dmitry Baryshkov wrote:
> Next commits are going to add support for additional RC parameter lookup
> tables. These tables are going to use different bpp/bpc combinations,
> thus it makes little sense to keep the 2d array for RC parameters.
> Switch to using the flat array.
>
> Signed-off-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/display/drm_dsc_helper.c | 188 +++
> 1 file changed, 88 insertions(+), 100 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c
> b/drivers/gpu/drm/display/drm_dsc_helper.c
> index deaa84722bd4..a6d11f474656 100644
> --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> @@ -307,24 +307,6 @@ void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config
> *vdsc_cfg)
> }
> EXPORT_SYMBOL(drm_dsc_set_rc_buf_thresh);
>
> -enum ROW_INDEX_BPP {
> - ROW_INDEX_6BPP = 0,
> - ROW_INDEX_8BPP,
> - ROW_INDEX_10BPP,
> - ROW_INDEX_12BPP,
> - ROW_INDEX_15BPP,
> - MAX_ROW_INDEX
> -};
> -
> -enum COLUMN_INDEX_BPC {
> - COLUMN_INDEX_8BPC = 0,
> - COLUMN_INDEX_10BPC,
> - COLUMN_INDEX_12BPC,
> - COLUMN_INDEX_14BPC,
> - COLUMN_INDEX_16BPC,
> - MAX_COLUMN_INDEX
> -};
> -
> struct rc_parameters {
> u16 initial_xmit_delay;
> u8 first_line_bpg_offset;
> @@ -336,12 +318,20 @@ struct rc_parameters {
> struct drm_dsc_rc_range_parameters rc_range_params[DSC_NUM_BUF_RANGES];
> };
>
> +struct rc_parameters_data {
> + u8 bpp;
> + u8 bpc;
> + struct rc_parameters params;
> +};
> +
> +#define DSC_BPP(bpp) ((bpp) << 4)
> +
> /*
> * Selected Rate Control Related Parameter Recommended Values
> * from DSC_v1.11 spec & C Model release: DSC_model_20161212
> */
> -static const struct rc_parameters rc_parameters[][MAX_COLUMN_INDEX] = {
> -{
> +static const struct rc_parameters_data rc_parameters[] = {
> +{ DSC_BPP(6), 8,
I was kind of hoping for a patch that would clean up the hideous
indentation in the tables. Please at least let's not add more with the
one space indent?
> /* 6BPP/8BPC */
With designated initializers I think we could just toss the comments
out.
.bpp = DSC_BPP(6), .bpc = 8,
With that,
Reviewed-by: Jani Nikula
> { 768, 15, 6144, 3, 13, 11, 11, {
> { 0, 4, 0 }, { 1, 6, -2 }, { 3, 8, -2 }, { 4, 8, -4 },
> @@ -349,7 +339,9 @@ static const struct rc_parameters
> rc_parameters[][MAX_COLUMN_INDEX] = {
> { 7, 11, -8 }, { 8, 12, -10 }, { 9, 12, -10 }, { 10, 12, -12 },
> { 10, 12, -12 }, { 11, 12, -12 }, { 13, 14, -12 }
> }
> - },
> + }
> +},
> +{ DSC_BPP(6), 10,
> /* 6BPP/10BPC */
> { 768, 15, 6144, 7, 17, 15, 15, {
> { 0, 8, 0 }, { 3, 10, -2 }, { 7, 12, -2 }, { 8, 12, -4 },
> @@ -358,7 +350,9 @@ static const struct rc_parameters
> rc_parameters[][MAX_COLUMN_INDEX] = {
> { 14, 16, -12 }, { 14, 16, -12 }, { 15, 16, -12 },
> { 17, 18, -12 }
> }
> - },
> + }
> +},
> +{ DSC_BPP(6), 12,
> /* 6BPP/12BPC */
> { 768, 15, 6144, 11, 21, 19, 19, {
> { 0, 12, 0 }, { 5, 14, -2 }, { 11, 16, -2 }, { 12, 16, -4 },
> @@ -367,7 +361,9 @@ static const struct rc_parameters
> rc_parameters[][MAX_COLUMN_INDEX] = {
> { 18, 20, -12 }, { 18, 20, -12 }, { 19, 20, -12 },
> { 21, 22, -12 }
> }
> - },
> + }
> +},
> +{ DSC_BPP(6), 14,
> /* 6BPP/14BPC */
> { 768, 15, 6144, 15, 25, 23, 23, {
> { 0, 16, 0 }, { 7, 18, -2 }, { 15, 20, -2 }, { 16, 20, -4 },
> @@ -376,7 +372,9 @@ static const struct rc_parameters
> rc_parameters[][MAX_COLUMN_INDEX] = {
> { 22, 24, -12 }, { 22, 24, -12 }, { 23, 24, -12 },
> { 25, 26, -12 }
> }
> - },
> + }
> +},
> +{ DSC_BPP(6), 16,
> /* 6BPP/16BPC */
> { 768, 15, 6144, 19, 29, 27, 27, {
> { 0, 20, 0 }, { 9, 22, -2 }, { 19, 24, -2 }, { 20, 24, -4 },
> @@ -385,9 +383,9 @@ static const struct rc_parameters
> rc_parameters[][MAX_COLUMN_INDEX] = {
> { 26, 28, -12 }, { 26, 28, -12 }, { 27, 28, -12 },
> { 29, 30, -12 }
> }
> - },
> + }
> },
> -{
> +{ DSC_BPP(8), 8,
> /* 8BPP/8BPC */
> { 512, 12, 6144, 3, 12, 11, 11, {
> { 0, 4, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
> @@ -395,7 +393,9 @@ static const struct rc_parameters
> rc_parameters[][MAX_COLUMN_INDEX] = {
> { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -
Re: [Freedreno] [PATCH 04/10] drm/i915/dsc: stop using interim structure for calculated params
rc->rc_range_params[buf_i].range_bpg_offset =
> + vdsc_cfg->rc_range_params[buf_i].range_bpg_offset =
> ofs_und6[buf_i]
> + res;
> } else if (bpp <= 12) {
> - rc->rc_range_params[buf_i].range_bpg_offset =
> + vdsc_cfg->rc_range_params[buf_i].range_bpg_offset =
> ofs_und8[buf_i];
> } else if (bpp <= 15) {
> res = DIV_ROUND_UP(((bpp - 12) * (ofs_und15[buf_i] -
> ofs_und12[buf_i])), 3);
> - rc->rc_range_params[buf_i].range_bpg_offset =
> + vdsc_cfg->rc_range_params[buf_i].range_bpg_offset =
>
> ofs_und12[buf_i] + res;
> } else {
> - rc->rc_range_params[buf_i].range_bpg_offset =
> + vdsc_cfg->rc_range_params[buf_i].range_bpg_offset =
>
> ofs_und15[buf_i];
This would benefit from a temp range_bpg_offset variable, assigned to
vdsc_cfg->rc_range_params[buf_i].range_bpg_offset after the if ladder.
> }
> }
> @@ -143,9 +131,6 @@ int intel_dsc_compute_params(struct intel_crtc_state
> *pipe_config)
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> struct drm_dsc_config *vdsc_cfg = _config->dsc.config;
> u16 compressed_bpp = pipe_config->dsc.compressed_bpp;
> - const struct rc_parameters *rc_params;
> - struct rc_parameters *rc = NULL;
> - u8 i = 0;
> int ret;
>
> vdsc_cfg->pic_width = pipe_config->hw.adjusted_mode.crtc_hdisplay;
> @@ -169,43 +154,12 @@ int intel_dsc_compute_params(struct intel_crtc_state
> *pipe_config)
>* parameters
>*/
> if (DISPLAY_VER(dev_priv) >= 13) {
> - rc = kmalloc(sizeof(*rc), GFP_KERNEL);
> - if (!rc)
> - return -ENOMEM;
> -
> - calculate_rc_params(rc, vdsc_cfg);
> - rc_params = rc;
> + calculate_rc_params(vdsc_cfg);
> } else {
> ret = drm_dsc_setup_rc_params(vdsc_cfg);
> if (ret)
> return ret;
>
> - goto out;
> - }
> -
> - vdsc_cfg->first_line_bpg_offset = rc_params->first_line_bpg_offset;
> - vdsc_cfg->initial_xmit_delay = rc_params->initial_xmit_delay;
> - vdsc_cfg->initial_offset = rc_params->initial_offset;
> - vdsc_cfg->flatness_min_qp = rc_params->flatness_min_qp;
> - vdsc_cfg->flatness_max_qp = rc_params->flatness_max_qp;
> - vdsc_cfg->rc_quant_incr_limit0 = rc_params->rc_quant_incr_limit0;
> - vdsc_cfg->rc_quant_incr_limit1 = rc_params->rc_quant_incr_limit1;
> -
> - for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
> - vdsc_cfg->rc_range_params[i].range_min_qp =
> - rc_params->rc_range_params[i].range_min_qp;
> - vdsc_cfg->rc_range_params[i].range_max_qp =
> - rc_params->rc_range_params[i].range_max_qp;
> - /*
> - * Range BPG Offset uses 2's complement and is only a 6 bits. So
> - * mask it to get only 6 bits.
> - */
> - vdsc_cfg->rc_range_params[i].range_bpg_offset =
> - rc_params->rc_range_params[i].range_bpg_offset &
> - DSC_RANGE_BPG_OFFSET_MASK;
This masking needs to be added to the loop in
calculate_rc_params(). With the temp variable I suggested above, it
could be done while assigning.
With that fixed,
Reviewed-by: Jani Nikula
Also, this is awesome stuff, thanks!
> - }
> -
> - if (DISPLAY_VER(dev_priv) < 13) {
> if (compressed_bpp == 6 &&
> vdsc_cfg->bits_per_component == 8)
> vdsc_cfg->rc_quant_incr_limit1 = 23;
> @@ -215,7 +169,6 @@ int intel_dsc_compute_params(struct intel_crtc_state
> *pipe_config)
> vdsc_cfg->rc_range_params[0].range_bpg_offset = 0;
> }
>
> -out:
> /*
>* BitsPerComponent value determines mux_word_size:
>* When BitsPerComponent is less than or 10bpc, muxWordSize will be
> equal to
> @@ -230,8 +183,6 @@ int intel_dsc_compute_params(struct intel_crtc_state
> *pipe_config)
> vdsc_cfg->initial_scale_value = (vdsc_cfg->rc_model_size << 3) /
> (vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset);
>
> - kfree(rc);
> -
> return 0;
> }
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH 03/10] drm/i915/dsc: move DSC tables to DRM DSC helper
PP/14BPC */
> + { 341, 15, 2048, 15, 24, 23, 23, {
> + { 0, 6, 2 }, { 7, 10, 0 }, { 9, 13, 0 }, { 11, 16, -2 },
> + { 14, 17, -4 }, { 15, 18, -6 }, { 15, 19, -8 }, { 15, 20, -8 },
> + { 15, 20, -8 }, { 15, 21, -10 }, { 17, 21, -10 },
> + { 17, 21, -12 }, { 17, 21, -12 }, { 19, 22, -12 },
> + { 22, 23, -12 }
> + }
> + },
> + /* 12BPP/16BPC */
> + { 341, 15, 2048, 19, 28, 27, 27, {
> + { 0, 6, 2 }, { 6, 11, 0 }, { 11, 15, 0 }, { 14, 18, -2 },
> + { 18, 21, -4 }, { 19, 22, -6 }, { 19, 23, -8 }, { 19, 24, -8 },
> + { 19, 24, -8 }, { 19, 25, -10 }, { 21, 25, -10 },
> + { 21, 25, -12 }, { 21, 25, -12 }, { 23, 26, -12 },
> + { 26, 27, -12 }
> + }
> + },
> +},
> +{
> + /* 15BPP/8BPC */
> + { 273, 15, 2048, 3, 12, 11, 11, {
> + { 0, 0, 10 }, { 0, 1, 8 }, { 0, 1, 6 }, { 0, 2, 4 },
> + { 1, 2, 2 }, { 1, 3, 0 }, { 1, 3, -2 }, { 2, 4, -4 },
> + { 2, 5, -6 }, { 3, 5, -8 }, { 4, 6, -10 }, { 4, 7, -10 },
> + { 5, 7, -12 }, { 7, 8, -12 }, { 8, 9, -12 }
> + }
> + },
> + /* 15BPP/10BPC */
> + { 273, 15, 2048, 7, 16, 15, 15, {
> + { 0, 2, 10 }, { 2, 5, 8 }, { 3, 5, 6 }, { 4, 6, 4 },
> + { 5, 6, 2 }, { 5, 7, 0 }, { 5, 7, -2 }, { 6, 8, -4 },
> + { 6, 9, -6 }, { 7, 9, -8 }, { 8, 10, -10 }, { 8, 11, -10 },
> + { 9, 11, -12 }, { 11, 12, -12 }, { 12, 13, -12 }
> + }
> + },
> + /* 15BPP/12BPC */
> + { 273, 15, 2048, 11, 20, 19, 19, {
> + { 0, 4, 10 }, { 2, 7, 8 }, { 4, 9, 6 }, { 6, 11, 4 },
> + { 9, 11, 2 }, { 9, 11, 0 }, { 9, 12, -2 }, { 10, 12, -4 },
> + { 11, 13, -6 }, { 11, 13, -8 }, { 12, 14, -10 },
> + { 13, 15, -10 }, { 13, 15, -12 }, { 15, 16, -12 },
> + { 16, 17, -12 }
> + }
> + },
> + /* 15BPP/14BPC */
> + { 273, 15, 2048, 15, 24, 23, 23, {
> + { 0, 4, 10 }, { 3, 8, 8 }, { 6, 11, 6 }, { 9, 14, 4 },
> + { 13, 15, 2 }, { 13, 15, 0 }, { 13, 16, -2 }, { 14, 16, -4 },
> + { 15, 17, -6 }, { 15, 17, -8 }, { 16, 18, -10 },
> + { 17, 19, -10 }, { 17, 19, -12 }, { 19, 20, -12 },
> + { 20, 21, -12 }
> + }
> + },
> + /* 15BPP/16BPC */
> + { 273, 15, 2048, 19, 28, 27, 27, {
> + { 0, 4, 10 }, { 4, 9, 8 }, { 8, 13, 6 }, { 12, 17, 4 },
> + { 17, 19, 2 }, { 17, 20, 0 }, { 17, 20, -2 }, { 18, 20, -4 },
> + { 19, 21, -6 }, { 19, 21, -8 }, { 20, 22, -10 },
> + { 21, 23, -10 }, { 21, 23, -12 }, { 23, 24, -12 },
> + { 24, 25, -12 }
> + }
> + }
> +}
> +};
> +
> +static int get_row_index_for_rc_params(u16 compressed_bpp)
> +{
> + switch (compressed_bpp) {
> + case 6:
> + return ROW_INDEX_6BPP;
> + case 8:
> + return ROW_INDEX_8BPP;
> + case 10:
> + return ROW_INDEX_10BPP;
> + case 12:
> + return ROW_INDEX_12BPP;
> + case 15:
> + return ROW_INDEX_15BPP;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int get_column_index_for_rc_params(u8 bits_per_component)
> +{
> + switch (bits_per_component) {
> + case 8:
> + return COLUMN_INDEX_8BPC;
> + case 10:
> + return COLUMN_INDEX_10BPC;
> + case 12:
> + return COLUMN_INDEX_12BPC;
> + case 14:
> + return COLUMN_INDEX_14BPC;
> + case 16:
> + return COLUMN_INDEX_16BPC;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct rc_parameters *get_rc_params(u16 compressed_bpp,
> + u8 bits_per_component)
> +{
> + int row_index, column_index;
> +
> + row_index = get_row_index_for_rc_params(compressed_bpp);
> + if (row_index < 0)
> + return NULL;
> +
> + column_index = get_column_index_for_rc_params(bits_per_component);
> + if (column_index < 0)
> + return NULL;
> +
> + return _parameters[row_index][column_index];
> +}
> +
> +/**
> + * drm_dsc_compute_rc_parameters() - Set parameters and limits for RC model
> in
Copy-paste function name left in. ;)
> + * accordance with the DSC 1.1 or 1.2 specification and DSC C Model
> + *
> + * @vdsc_cfg: DSC Configuration data partially filled by driver
Maybe mention what needs to be set beforehands? Or better yet, warn
about missing setup and
Re: [Freedreno] [PATCH 02/10] drm/i915/dsc: move rc_buf_thresh values to common helper
On Tue, 28 Feb 2023, Dmitry Baryshkov wrote:
> The rc_buf_thresh values are common to all DSC implementations. Move
> them to the common helper together with the code to propagage them to
> the drm_dsc_config.
>
> Signed-off-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/display/drm_dsc_helper.c | 37 +++
> drivers/gpu/drm/i915/display/intel_vdsc.c | 24 +--
> include/drm/display/drm_dsc_helper.h | 1 +
> 3 files changed, 39 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c
> b/drivers/gpu/drm/display/drm_dsc_helper.c
> index c869c6e51e2b..ab8679c158b5 100644
> --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> @@ -270,6 +270,43 @@ void drm_dsc_pps_payload_pack(struct
> drm_dsc_picture_parameter_set *pps_payload,
> }
> EXPORT_SYMBOL(drm_dsc_pps_payload_pack);
>
> +/* From DSC_v1.11 spec, rc_parameter_Set syntax element typically constant */
> +const u16 drm_dsc_rc_buf_thresh[] = {
> + 896, 1792, 2688, 3584, 4480, 5376, 6272, 6720, 7168, 7616,
> + 7744, 7872, 8000, 8064
> +};
> +EXPORT_SYMBOL(drm_dsc_rc_buf_thresh);
> +
> +/**
> + * drm_dsc_set_rc_buf_thresh() - Set thresholds for the RC model
> + * in accordance with the DSC 1.2 specification.
> + *
> + * @vdsc_cfg: DSC Configuration data partially filled by driver
> + */
> +void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg)
> +{
> + int i = 0;
> +
> + for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) {
> + /*
> + * six 0s are appended to the lsb of each threshold value
> + * internally in h/w.
> + * Only 8 bits are allowed for programming RcBufThreshold
> + */
Not sure how appropriate the hardware references are, maybe clean it up
a bit.
With that, and +static and -export mentioned earlier,
Reviewed-by: Jani Nikula
> + vdsc_cfg->rc_buf_thresh[i] = drm_dsc_rc_buf_thresh[i] >> 6;
> + }
> +
> + /*
> + * For 6bpp, RC Buffer threshold 12 and 13 need a different value
> + * as per C Model
> + */
> + if (vdsc_cfg->bits_per_pixel == 6 << 4) {
> + vdsc_cfg->rc_buf_thresh[12] = 7936 >> 6;
> + vdsc_cfg->rc_buf_thresh[13] = 8000 >> 6;
> + }
> +}
> +EXPORT_SYMBOL(drm_dsc_set_rc_buf_thresh);
> +
> /**
> * drm_dsc_compute_rc_parameters() - Write rate control
> * parameters to the dsc configuration defined in
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index d080741fd0b3..b4faab4c8fb3 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -36,12 +36,6 @@ enum COLUMN_INDEX_BPC {
> MAX_COLUMN_INDEX
> };
>
> -/* From DSC_v1.11 spec, rc_parameter_Set syntax element typically constant */
> -static const u16 rc_buf_thresh[] = {
> - 896, 1792, 2688, 3584, 4480, 5376, 6272, 6720, 7168, 7616,
> - 7744, 7872, 8000, 8064
> -};
> -
> struct rc_parameters {
> u16 initial_xmit_delay;
> u8 first_line_bpg_offset;
> @@ -474,23 +468,7 @@ int intel_dsc_compute_params(struct intel_crtc_state
> *pipe_config)
> vdsc_cfg->bits_per_pixel = compressed_bpp << 4;
> vdsc_cfg->bits_per_component = pipe_config->pipe_bpp / 3;
>
> - for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) {
> - /*
> - * six 0s are appended to the lsb of each threshold value
> - * internally in h/w.
> - * Only 8 bits are allowed for programming RcBufThreshold
> - */
> - vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
> - }
> -
> - /*
> - * For 6bpp, RC Buffer threshold 12 and 13 need a different value
> - * as per C Model
> - */
> - if (compressed_bpp == 6) {
> - vdsc_cfg->rc_buf_thresh[12] = 0x7C;
> - vdsc_cfg->rc_buf_thresh[13] = 0x7D;
> - }
> + drm_dsc_set_rc_buf_thresh(vdsc_cfg);
>
> /*
>* From XE_LPD onwards we supports compression bpps in steps of 1
> diff --git a/include/drm/display/drm_dsc_helper.h
> b/include/drm/display/drm_dsc_helper.h
> index 8b41edbbabab..706ba1d34742 100644
> --- a/include/drm/display/drm_dsc_helper.h
> +++ b/include/drm/display/drm_dsc_helper.h
> @@ -14,6 +14,7 @@ void drm_dsc_dp_pps_header_init(struct dp_sdp_header
> *pps_header);
> int drm_dsc_dp_rc_buffer_size(u8 rc_buffer_block_size, u8 rc_buffer_size);
> void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_sdp,
> const struct drm_dsc_config *dsc_cfg);
> +void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg);
> int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
>
> #endif /* _DRM_DSC_HELPER_H_ */
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH 01/10] drm/i915/dsc: change DSC param tables to follow the DSC model
On Tue, 28 Feb 2023, Dmitry Baryshkov wrote:
> After cross-checking DSC models (20150914, 20161212, 20210623) change
> values in rc_parameters tables to follow config files present inside
> the DSC model. Handle two places, where i915 tables diverged from the
> model, by patching the rc values in the code.
>
> Note: I left one case uncorrected, 8bpp/10bpc/range_max_qp[0], because
> the table in the VESA DSC 1.1 sets it to 4.
>
> Signed-off-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/i915/display/intel_vdsc.c | 18 --
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index 207b2a648d32..d080741fd0b3 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -86,7 +86,7 @@ static const struct rc_parameters
> rc_parameters[][MAX_COLUMN_INDEX] = {
> }
> },
> /* 6BPP/14BPC */
> - { 768, 15, 6144, 15, 25, 23, 27, {
> + { 768, 15, 6144, 15, 25, 23, 23, {
> { 0, 16, 0 }, { 7, 18, -2 }, { 15, 20, -2 }, { 16, 20, -4 },
> { 17, 21, -6 }, { 17, 21, -6 }, { 18, 21, -6 }, { 18, 22, -8 },
> { 19, 23, -8 }, { 20, 24, -10 }, { 21, 24, -10 },
> @@ -115,6 +115,10 @@ static const struct rc_parameters
> rc_parameters[][MAX_COLUMN_INDEX] = {
> },
> /* 8BPP/10BPC */
> { 512, 12, 6144, 7, 16, 15, 15, {
> + /*
> + * DSC model/pre-SCR-cfg has 8 for range_max_qp[0], however
> + * VESA DSC 1.1 Table E-5 sets it to 4.
> + */
> { 0, 4, 2 }, { 4, 8, 0 }, { 5, 9, 0 }, { 5, 10, -2 },
> { 7, 11, -4 }, { 7, 11, -6 }, { 7, 11, -8 }, { 7, 12, -8 },
> { 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9, 16, -12 },
> @@ -132,7 +136,7 @@ static const struct rc_parameters
> rc_parameters[][MAX_COLUMN_INDEX] = {
> },
> /* 8BPP/14BPC */
> { 512, 12, 6144, 15, 24, 23, 23, {
> - { 0, 12, 0 }, { 5, 13, 0 }, { 11, 15, 0 }, { 12, 17, -2 },
> + { 0, 12, 2 }, { 5, 13, 0 }, { 11, 15, 0 }, { 12, 17, -2 },
> { 15, 19, -4 }, { 15, 19, -6 }, { 15, 19, -8 }, { 15, 20, -8 },
> { 15, 21, -8 }, { 15, 22, -10 }, { 17, 22, -10 },
> { 17, 23, -12 }, { 17, 23, -12 }, { 21, 24, -12 },
> @@ -529,6 +533,16 @@ int intel_dsc_compute_params(struct intel_crtc_state
> *pipe_config)
> DSC_RANGE_BPG_OFFSET_MASK;
> }
>
> + if (DISPLAY_VER(dev_priv) < 13) {
> + if (compressed_bpp == 6 &&
> + vdsc_cfg->bits_per_component == 8)
> + vdsc_cfg->rc_quant_incr_limit1 = 23;
> +
> + if (compressed_bpp == 8 &&
> + vdsc_cfg->bits_per_component == 14)
> + vdsc_cfg->rc_range_params[0].range_bpg_offset = 0;
> + }
> +
I wonder if we shouldn't just use the updated values...
Maybe add a FIXME comment above the block to consider removing it?
Reviewed-by: Jani Nikula
> /*
>* BitsPerComponent value determines mux_word_size:
>* When BitsPerComponent is less than or 10bpc, muxWordSize will be
> equal to
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH 02/10] drm/i915/dsc: move rc_buf_thresh values to common helper
On Tue, 28 Feb 2023, Dmitry Baryshkov wrote:
> On Tue, 28 Feb 2023 at 14:25, Jani Nikula wrote:
>>
>> On Tue, 28 Feb 2023, Dmitry Baryshkov wrote:
>> > The rc_buf_thresh values are common to all DSC implementations. Move
>> > them to the common helper together with the code to propagage them to
>> > the drm_dsc_config.
>> >
>> > Signed-off-by: Dmitry Baryshkov
>> > ---
>> > drivers/gpu/drm/display/drm_dsc_helper.c | 37 +++
>> > drivers/gpu/drm/i915/display/intel_vdsc.c | 24 +--
>> > include/drm/display/drm_dsc_helper.h | 1 +
>> > 3 files changed, 39 insertions(+), 23 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c
>> > b/drivers/gpu/drm/display/drm_dsc_helper.c
>> > index c869c6e51e2b..ab8679c158b5 100644
>> > --- a/drivers/gpu/drm/display/drm_dsc_helper.c
>> > +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
>> > @@ -270,6 +270,43 @@ void drm_dsc_pps_payload_pack(struct
>> > drm_dsc_picture_parameter_set *pps_payload,
>> > }
>> > EXPORT_SYMBOL(drm_dsc_pps_payload_pack);
>> >
>> > +/* From DSC_v1.11 spec, rc_parameter_Set syntax element typically
>> > constant */
>> > +const u16 drm_dsc_rc_buf_thresh[] = {
>> > + 896, 1792, 2688, 3584, 4480, 5376, 6272, 6720, 7168, 7616,
>> > + 7744, 7872, 8000, 8064
>> > +};
>> > +EXPORT_SYMBOL(drm_dsc_rc_buf_thresh);
>>
>> This needs to be static, without exports.
>
> Exported this to let other drivers use it, while skipping the
> drm_dsc_set_rc_buf_thresh(). For example amdgpu driver sets buffer
> thresholds on the interim structure, so the helper is not directly
> applicable. See _do_calc_rc_params().
Regardless, I'm still saying don't do that.
Data is not an interface.
If you make it easy to just use the data, nobody will ever fix their
drivers to use proper interfaces, and you'll lock yourself to a
particular representation of the data even though it's supposed to be a
hidden implementation detail.
BR,
Jani.
>
>>
>> > +
>> > +/**
>> > + * drm_dsc_set_rc_buf_thresh() - Set thresholds for the RC model
>> > + * in accordance with the DSC 1.2 specification.
>> > + *
>> > + * @vdsc_cfg: DSC Configuration data partially filled by driver
>> > + */
>> > +void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg)
>> > +{
>> > + int i = 0;
>>
>> Unnecessary initialization.
>
> My bad.
>
>>
>> > +
>> > + for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) {
>>
>> Please use ARRAY_SIZE(). Maybe add BUILD_BUG_ON() for DSC_NUM_BUF_RANGES
>> vs. ARRAY_SIZE(). (Yes, we should've used ARRAY_SIZE() in i915.)
>
> Ack
>
>>
>> > + /*
>> > + * six 0s are appended to the lsb of each threshold value
>> > + * internally in h/w.
>> > + * Only 8 bits are allowed for programming RcBufThreshold
>> > + */
>> > + vdsc_cfg->rc_buf_thresh[i] = drm_dsc_rc_buf_thresh[i] >> 6;
>> > + }
>> > +
>> > + /*
>> > + * For 6bpp, RC Buffer threshold 12 and 13 need a different value
>> > + * as per C Model
>> > + */
>> > + if (vdsc_cfg->bits_per_pixel == 6 << 4) {
>> > + vdsc_cfg->rc_buf_thresh[12] = 7936 >> 6;
>> > + vdsc_cfg->rc_buf_thresh[13] = 8000 >> 6;
>> > + }
>> > +}
>> > +EXPORT_SYMBOL(drm_dsc_set_rc_buf_thresh);
>> > +
>> > /**
>> > * drm_dsc_compute_rc_parameters() - Write rate control
>> > * parameters to the dsc configuration defined in
>> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> > b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> > index d080741fd0b3..b4faab4c8fb3 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> > @@ -36,12 +36,6 @@ enum COLUMN_INDEX_BPC {
>> > MAX_COLUMN_INDEX
>> > };
>> >
>> > -/* From DSC_v1.11 spec, rc_parameter_Set syntax element typically
>> > constant */
>> > -static const u16 rc_buf_thresh[] = {
>> > - 896, 1792, 2688, 3584, 4480, 5376, 6272, 6720, 7168, 7616,
>> > - 7744, 7872, 8000, 8064
>> > -};
>> > -
>> > struct rc_parameters {
>> > u16 initial_xmit_delay;
>> >
Re: [Freedreno] [PATCH 02/10] drm/i915/dsc: move rc_buf_thresh values to common helper
On Tue, 28 Feb 2023, Dmitry Baryshkov wrote:
> The rc_buf_thresh values are common to all DSC implementations. Move
> them to the common helper together with the code to propagage them to
> the drm_dsc_config.
>
> Signed-off-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/display/drm_dsc_helper.c | 37 +++
> drivers/gpu/drm/i915/display/intel_vdsc.c | 24 +--
> include/drm/display/drm_dsc_helper.h | 1 +
> 3 files changed, 39 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c
> b/drivers/gpu/drm/display/drm_dsc_helper.c
> index c869c6e51e2b..ab8679c158b5 100644
> --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> @@ -270,6 +270,43 @@ void drm_dsc_pps_payload_pack(struct
> drm_dsc_picture_parameter_set *pps_payload,
> }
> EXPORT_SYMBOL(drm_dsc_pps_payload_pack);
>
> +/* From DSC_v1.11 spec, rc_parameter_Set syntax element typically constant */
> +const u16 drm_dsc_rc_buf_thresh[] = {
> + 896, 1792, 2688, 3584, 4480, 5376, 6272, 6720, 7168, 7616,
> + 7744, 7872, 8000, 8064
> +};
> +EXPORT_SYMBOL(drm_dsc_rc_buf_thresh);
This needs to be static, without exports.
> +
> +/**
> + * drm_dsc_set_rc_buf_thresh() - Set thresholds for the RC model
> + * in accordance with the DSC 1.2 specification.
> + *
> + * @vdsc_cfg: DSC Configuration data partially filled by driver
> + */
> +void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg)
> +{
> + int i = 0;
Unnecessary initialization.
> +
> + for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) {
Please use ARRAY_SIZE(). Maybe add BUILD_BUG_ON() for DSC_NUM_BUF_RANGES
vs. ARRAY_SIZE(). (Yes, we should've used ARRAY_SIZE() in i915.)
> + /*
> + * six 0s are appended to the lsb of each threshold value
> + * internally in h/w.
> + * Only 8 bits are allowed for programming RcBufThreshold
> + */
> + vdsc_cfg->rc_buf_thresh[i] = drm_dsc_rc_buf_thresh[i] >> 6;
> + }
> +
> + /*
> + * For 6bpp, RC Buffer threshold 12 and 13 need a different value
> + * as per C Model
> + */
> + if (vdsc_cfg->bits_per_pixel == 6 << 4) {
> + vdsc_cfg->rc_buf_thresh[12] = 7936 >> 6;
> + vdsc_cfg->rc_buf_thresh[13] = 8000 >> 6;
> + }
> +}
> +EXPORT_SYMBOL(drm_dsc_set_rc_buf_thresh);
> +
> /**
> * drm_dsc_compute_rc_parameters() - Write rate control
> * parameters to the dsc configuration defined in
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index d080741fd0b3..b4faab4c8fb3 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -36,12 +36,6 @@ enum COLUMN_INDEX_BPC {
> MAX_COLUMN_INDEX
> };
>
> -/* From DSC_v1.11 spec, rc_parameter_Set syntax element typically constant */
> -static const u16 rc_buf_thresh[] = {
> - 896, 1792, 2688, 3584, 4480, 5376, 6272, 6720, 7168, 7616,
> - 7744, 7872, 8000, 8064
> -};
> -
> struct rc_parameters {
> u16 initial_xmit_delay;
> u8 first_line_bpg_offset;
> @@ -474,23 +468,7 @@ int intel_dsc_compute_params(struct intel_crtc_state
> *pipe_config)
> vdsc_cfg->bits_per_pixel = compressed_bpp << 4;
> vdsc_cfg->bits_per_component = pipe_config->pipe_bpp / 3;
>
> - for (i = 0; i < DSC_NUM_BUF_RANGES - 1; i++) {
> - /*
> - * six 0s are appended to the lsb of each threshold value
> - * internally in h/w.
> - * Only 8 bits are allowed for programming RcBufThreshold
> - */
> - vdsc_cfg->rc_buf_thresh[i] = rc_buf_thresh[i] >> 6;
> - }
> -
> - /*
> - * For 6bpp, RC Buffer threshold 12 and 13 need a different value
> - * as per C Model
> - */
> - if (compressed_bpp == 6) {
> - vdsc_cfg->rc_buf_thresh[12] = 0x7C;
> - vdsc_cfg->rc_buf_thresh[13] = 0x7D;
> - }
> + drm_dsc_set_rc_buf_thresh(vdsc_cfg);
>
> /*
>* From XE_LPD onwards we supports compression bpps in steps of 1
> diff --git a/include/drm/display/drm_dsc_helper.h
> b/include/drm/display/drm_dsc_helper.h
> index 8b41edbbabab..706ba1d34742 100644
> --- a/include/drm/display/drm_dsc_helper.h
> +++ b/include/drm/display/drm_dsc_helper.h
> @@ -14,6 +14,7 @@ void drm_dsc_dp_pps_header_init(struct dp_sdp_header
> *pps_header);
> int drm_dsc_dp_rc_buffer_size(u8 rc_buffer_block_size, u8 rc_buffer_size);
> void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_sdp,
> const struct drm_dsc_config *dsc_cfg);
> +void drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg);
> int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
>
> #endif /* _DRM_DSC_HELPER_H_ */
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [RFC PATCH 1/2] drm/msm/dpu: add dsc helper functions
;> drm_dsc_config struct via the generic helper. >> >> >>> So if this has any merit and if you or Marijn would like to take it >>> up, go for it. We would do the same thing as either of you would have >>> to in terms of figuring out the difference between msm and the i915 code. >>> >>> This is not a generic API we are trying to put in a helper, these are >>> hard-coded tables so there is a difference between looking at these Vs >>> looking at some common code which can move to the core. >>> >>>>> >>>>>>> >>>>>>> Also, i think its too risky to change other drivers to use >>>>>>> whatever math >>>>>>> we put in the drm_dsc_helper to compute thr RC params because >>>>>>> their code >>>>>>> might be computing and using this tables differently. >>>>>>> >>>>>>> Its too much ownership for MSM developers to move this to >>>>>>> drm_dsc_helper >>>>>>> and own that as it might cause breakage of basic DSC even if some >>>>>>> values >>>>>>> are repeated. >>>>>> >>>>>> It's time to stop thinking about ownership and start thinking about >>>>>> shared code. We already have two instances of DSC tables. I don't >>>>>> think having a third instance, which is a subset of an existing >>>>>> dataset, would be beneficial to anybody. >>>>>> AMD has complicated code which supports half-bit bpp and calculates >>>>>> some of the parameters. But sharing data with the i915 driver is >>>>>> straightforward. >>>>>> >>>>> >>>>> Sorry, but I would like to get an ack from i915 folks if this is going >>>>> to be useful to them if we move this to helper because we have to >>>>> look at every table. Not just one. >>>> >>>> Added i915 maintainers to the CC list for them to be able to answer. >>>> >>> >>> Thanks, lets wait to hear from them about where finally these tables >>> should go but thats can be taken up as a separate effort too. >>> >>>>> >>>>> Also, this is just 1.1, we will add more tables for 1.2. So we will >>>>> have to end up changing both 1.1 and 1.2 tables as they are >>>>> different for QC. >>>> >>>> I haven't heard back from Kuogee about the possible causes of using >>>> rc/qp values from 1.2 even for 1.1 panels. Maybe you can comment on >>>> that? In other words, can we always stick to the values from 1.2 >>>> standard? What will be the drawback? >>>> >>>> Otherwise, we'd have to have two different sets of values, like you >>>> do in your vendor driver. >>>> >>> >>> I have responded to this in the other email. >>> >>> All this being said, even if the rc tables move the drm_dsc_helper >>> either now or later on, we will still need MSM specific calculations >>> for many of the other encoder parameters (which are again either >>> hard-coded or calculated). Please refer to the >>> sde_dsc_populate_dsc_config() downstream. And yes, you will not find >>> those in the DP spec directly. >>> >>> So we will still need a dsc helper for MSM calculations to be common >>> for DSI / DP irrespective of where the tables go. >>> >>> So, lets finalize that first. >> >> I went on and trimmed sde_dsc_populate_dsc_config() to remove >> duplication with the drm_dsc_compute_rc_parameters() (which we already >> use for the MSM DSI DSC). >> >> Not much is left: >> >> dsc->first_line_bpg_offset set via the switch >> >> dsc->line_buf_depth = bpc + 1; >> dsc->mux_word_size = bpc > 10 ? DSC_MUX_WORD_SIZE_12_BPC: >> DSC_MUX_WORD_SIZE_8_10_BPC; >> >> if ((dsc->dsc_version_minor == 0x2) && (dsc->native_420)) >> dsc->nsl_bpg_offset = (2048 * >> (DIV_ROUND_UP(dsc->second_line_bpg_offset, >> (dsc->slice_height - 1; >> >> dsc->initial_scale_value = 8 * dsc->rc_model_size / >> (dsc->rc_model_size - dsc->initial_offset); >> >> >> mux_word_size comes from the standard (must) >> initial_scale_value calculation is recommended, but not required >> nsl_bpg_offset follows the standard (must), also see below (*). >> >> first_line_bpg_offset calculation differs between three drivers. The >> standard also provides a recommended formulas. I think we can leave it >> as is for now. >> >> I think, that mux_word_size and nsl_bpg_offset calculation should be >> moved to drm_dsc_compute_rc_parameters(), while leaving >> initial_scale_value in place (in the driver code). >> >> * I think nsl_bpg_offset is slightly incorrectly calculated. Standard >> demands that it is set to 'second_line_bpg_offset / (slice_height - 1), >> rounded up to 16 fraction bits', while SDE driver code sets it to the >> value rounded up to the next integer (having 16 fraction bits >> representation). >> >> In my opinion correct calculation should be: >> dsc->nsl_bpg_offset = DIV_ROUND_UP(2048 * dsc->second_line_bpg_offset, >> (dsc->slice_height - 1)); >> >> Could you please check, which one is correct according to the standard? >> >> > > Sure, i will check about nsl_bpg_offset. But sorry if I was not more > clear about this but sde_dsc_populate_dsc_config() is only one example > which from your analysis can be moved to the drm_dsc_helper() but not > the initial line calculation _dce_dsc_initial_line_calc(), > _dce_dsc_ich_reset_override_needed() , _dce_dsc_setup_helper(). > > All of these are again common between DSI and DP. > > So in addition to thinking about what can be moved to the drm_dsc_helper > also think about what is specific to MSM but common to DSI and DP modules. > > That was the bigger picture I was trying to convey. -- Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [Intel-gfx] [PATCH 3/3] drm/connector: move ELD and video/audio latencies to display info
Obviously, I need to still work on this. *looks for brown paper bag*
On Tue, 24 Jan 2023, kernel test robot wrote:
> Hi Jani,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on drm-tip/drm-tip]
>
> url:
> https://github.com/intel-lab-lkp/linux/commits/Jani-Nikula/drm-connector-move-HDR-sink-metadata-to-display-info/20230124-174322
> base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
> patch link:
> https://lore.kernel.org/r/20230124094154.2282778-3-jani.nikula%40intel.com
> patch subject: [Intel-gfx] [PATCH 3/3] drm/connector: move ELD and
> video/audio latencies to display info
> config: arm-defconfig
> (https://download.01.org/0day-ci/archive/20230124/202301242049.ekzx7rzz-...@intel.com/config)
> compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
> reproduce (this is a W=1 build):
> wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
> ~/bin/make.cross
> chmod +x ~/bin/make.cross
> #
> https://github.com/intel-lab-lkp/linux/commit/1e92b5478cfc1b0df6615365217e9548b0d5
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review
> Jani-Nikula/drm-connector-move-HDR-sink-metadata-to-display-info/20230124-174322
> git checkout 1e92b5478cfc1b0df6615365217e9548b0d5
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1
> O=build_dir ARCH=arm olddefconfig
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1
> O=build_dir ARCH=arm SHELL=/bin/bash drivers/gpu/
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot
>
> All errors (new ones prefixed by >>):
>
>drivers/gpu/drm/tegra/hdmi.c: In function 'tegra_hdmi_write_eld':
>>> drivers/gpu/drm/tegra/hdmi.c:620:60: error: 'struct drm_connector' has no
>>> member named 'eld'
> 620 | size_t length = drm_eld_size(hdmi->output.connector.eld),
> i;
> |^
>drivers/gpu/drm/tegra/hdmi.c:624:72: error: 'struct drm_connector' has no
> member named 'eld'
> 624 | tegra_hdmi_writel(hdmi, i << 8 |
> hdmi->output.connector.eld[i],
> |
> ^
> --
>drivers/gpu/drm/tegra/sor.c: In function 'tegra_sor_write_eld':
>>> drivers/gpu/drm/tegra/sor.c:1951:59: error: 'struct drm_connector' has no
>>> member named 'eld'
> 1951 | size_t length = drm_eld_size(sor->output.connector.eld), i;
> | ^
>drivers/gpu/drm/tegra/sor.c:1954:69: error: 'struct drm_connector' has no
> member named 'eld'
> 1954 | tegra_sor_writel(sor, i << 8 |
> sor->output.connector.eld[i],
> |
> ^
>
>
> vim +620 drivers/gpu/drm/tegra/hdmi.c
>
> edec4af4c3d6d2 Thierry Reding 2012-11-15 617
> 5234549b93aa2a Thierry Reding 2015-08-07 618 static void
> tegra_hdmi_write_eld(struct tegra_hdmi *hdmi)
> 5234549b93aa2a Thierry Reding 2015-08-07 619 {
> 5234549b93aa2a Thierry Reding 2015-08-07 @620 size_t length =
> drm_eld_size(hdmi->output.connector.eld), i;
> 5234549b93aa2a Thierry Reding 2015-08-07 621 u32 value;
> edec4af4c3d6d2 Thierry Reding 2012-11-15 622
> 5234549b93aa2a Thierry Reding 2015-08-07 623 for (i = 0; i < length;
> i++)
> 5234549b93aa2a Thierry Reding 2015-08-07 624
> tegra_hdmi_writel(hdmi, i << 8 | hdmi->output.connector.eld[i],
> 5234549b93aa2a Thierry Reding 2015-08-07 625
> HDMI_NV_PDISP_SOR_AUDIO_HDA_ELD_BUFWR);
> edec4af4c3d6d2 Thierry Reding 2012-11-15 626
> 5234549b93aa2a Thierry Reding 2015-08-07 627 /*
> 5234549b93aa2a Thierry Reding 2015-08-07 628 * The HDA codec will
> always report an ELD buffer size of 96 bytes and
> 5234549b93aa2a Thierry Reding 2015-08-07 629 * the HDA codec driver
> will check that each byte read from the buffer
> 5234549b93aa2a Thierry Reding 2015-08-07 630 * is valid. Therefore
> every byte must be written, even if no 96 bytes
> 5234549b93aa2a Thierry Reding 2015-08-07 631 * were parsed from
> EDID.
> 5234549b93aa2a Thierry Reding 2015-08-07 632 */
> 5234549b93aa2a Thierry Reding 2015-08-07 633 for (i = length; i <
> HDMI
Re: [Freedreno] [PATCH v3 1/2] drm/probe_helper: extract two helper functions
On Tue, 24 Jan 2023, Dmitry Baryshkov wrote:
> Extract drm_kms_helper_enable_hpd() and drm_kms_helper_disable_hpd(),
> two helpers that enable and disable HPD handling on all device's
> connectors.
Thanks for separating this, it makes the other patch easier to read.
Reviewed-by: Jani Nikula
>
> Signed-off-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/drm_probe_helper.c | 68 ++
> 1 file changed, 41 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> b/drivers/gpu/drm/drm_probe_helper.c
> index 95aeeed33cf5..ab787d71fa66 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -222,6 +222,45 @@ drm_connector_mode_valid(struct drm_connector *connector,
> return ret;
> }
>
> +static void drm_kms_helper_disable_hpd(struct drm_device *dev)
> +{
> + struct drm_connector *connector;
> + struct drm_connector_list_iter conn_iter;
> +
> + drm_connector_list_iter_begin(dev, _iter);
> + drm_for_each_connector_iter(connector, _iter) {
> + const struct drm_connector_helper_funcs *funcs =
> + connector->helper_private;
> +
> + if (funcs && funcs->disable_hpd)
> + funcs->disable_hpd(connector);
> + }
> + drm_connector_list_iter_end(_iter);
> +}
> +
> +static bool drm_kms_helper_enable_hpd(struct drm_device *dev)
> +{
> + bool poll = false;
> + struct drm_connector *connector;
> + struct drm_connector_list_iter conn_iter;
> +
> + drm_connector_list_iter_begin(dev, _iter);
> + drm_for_each_connector_iter(connector, _iter) {
> + const struct drm_connector_helper_funcs *funcs =
> + connector->helper_private;
> +
> + if (funcs && funcs->enable_hpd)
> + funcs->enable_hpd(connector);
> +
> + if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT |
> + DRM_CONNECTOR_POLL_DISCONNECT))
> + poll = true;
> + }
> + drm_connector_list_iter_end(_iter);
> +
> + return poll;
> +}
> +
> #define DRM_OUTPUT_POLL_PERIOD (10*HZ)
> /**
> * drm_kms_helper_poll_enable - re-enable output polling.
> @@ -241,26 +280,12 @@ drm_connector_mode_valid(struct drm_connector
> *connector,
> void drm_kms_helper_poll_enable(struct drm_device *dev)
> {
> bool poll = false;
> - struct drm_connector *connector;
> - struct drm_connector_list_iter conn_iter;
> unsigned long delay = DRM_OUTPUT_POLL_PERIOD;
>
> if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll)
> return;
>
> - drm_connector_list_iter_begin(dev, _iter);
> - drm_for_each_connector_iter(connector, _iter) {
> - const struct drm_connector_helper_funcs *funcs =
> - connector->helper_private;
> -
> - if (funcs && funcs->enable_hpd)
> - funcs->enable_hpd(connector);
> -
> - if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT |
> - DRM_CONNECTOR_POLL_DISCONNECT))
> - poll = true;
> - }
> - drm_connector_list_iter_end(_iter);
> + poll = drm_kms_helper_enable_hpd(dev);
>
> if (dev->mode_config.delayed_event) {
> /*
> @@ -810,24 +835,13 @@ EXPORT_SYMBOL(drm_kms_helper_is_poll_worker);
>
> static void drm_kms_helper_poll_disable_fini(struct drm_device *dev, bool
> fini)
> {
> - struct drm_connector *connector;
> - struct drm_connector_list_iter conn_iter;
> -
> if (!dev->mode_config.poll_enabled)
> return;
>
> if (fini)
> dev->mode_config.poll_enabled = false;
>
> - drm_connector_list_iter_begin(dev, _iter);
> - drm_for_each_connector_iter(connector, _iter) {
> - const struct drm_connector_helper_funcs *funcs =
> - connector->helper_private;
> -
> - if (funcs && funcs->disable_hpd)
> - funcs->disable_hpd(connector);
> - }
> - drm_connector_list_iter_end(_iter);
> + drm_kms_helper_disable_hpd(dev);
>
> cancel_delayed_work_sync(>mode_config.output_poll_work);
> }
--
Jani Nikula, Intel Open Source Graphics Center
[Freedreno] [PATCH 3/3] drm/connector: move ELD and video/audio latencies to display info
Information parsed from the display EDID should be stored in display
info. We can stop clearing ELD separately.
Cc: Alex Deucher
Cc: Christian König
Cc: Pan, Xinhui
Cc: amd-...@lists.freedesktop.org
Cc: Andrzej Hajda
Cc: Neil Armstrong
Cc: Robert Foss
Cc: Laurent Pinchart
Cc: Jonas Karlman
Cc: Jernej Skrabec
Cc: Inki Dae
Cc: Seung-Woo Kim
Cc: Kyungmin Park
Cc: Chun-Kuang Hu
Cc: Philipp Zabel
Cc: linux-media...@lists.infradead.org
Cc: Rob Clark
Cc: Abhinav Kumar
Cc: Dmitry Baryshkov
Cc: Sean Paul
Cc: linux-arm-...@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Cc: Alain Volmat
Cc: Emma Anholt
Cc: Maxime Ripard
Signed-off-by: Jani Nikula
---
Sorry about the crazy Cc list, but this touches a lot of drivers, and I
didn't want to blind side anyone.
---
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 6 +--
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 6 +--
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 6 +--
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 12 ++---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++---
drivers/gpu/drm/bridge/analogix/anx7625.c | 4 +-
drivers/gpu/drm/bridge/ite-it66121.c | 4 +-
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +-
drivers/gpu/drm/drm_edid.c| 45 +++
drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +-
drivers/gpu/drm/i915/display/intel_audio.c| 22 -
drivers/gpu/drm/i915/display/intel_sdvo.c | 2 +-
drivers/gpu/drm/mediatek/mtk_dp.c | 2 +-
drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 +-
drivers/gpu/drm/msm/dp/dp_audio.c | 4 +-
drivers/gpu/drm/radeon/dce6_afmt.c| 12 ++---
drivers/gpu/drm/radeon/evergreen_hdmi.c | 12 ++---
drivers/gpu/drm/radeon/radeon_audio.c | 4 +-
drivers/gpu/drm/sti/sti_hdmi.c| 2 +-
drivers/gpu/drm/vc4/vc4_hdmi.c| 2 +-
include/drm/drm_connector.h | 39 +---
21 files changed, 99 insertions(+), 102 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index 9a24ed463abd..0c05838032c5 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -1257,11 +1257,11 @@ static void dce_v10_0_audio_write_latency_fields(struct
drm_encoder *encoder,
if (mode->flags & DRM_MODE_FLAG_INTERLACE)
interlace = 1;
- if (connector->latency_present[interlace]) {
+ if (connector->display_info.latency_present[interlace]) {
tmp = REG_SET_FIELD(0,
AZALIA_F0_CODEC_PIN_CONTROL_RESPONSE_LIPSYNC,
- VIDEO_LIPSYNC,
connector->video_latency[interlace]);
+ VIDEO_LIPSYNC,
connector->display_info.video_latency[interlace]);
tmp = REG_SET_FIELD(0,
AZALIA_F0_CODEC_PIN_CONTROL_RESPONSE_LIPSYNC,
- AUDIO_LIPSYNC,
connector->audio_latency[interlace]);
+ AUDIO_LIPSYNC,
connector->display_info.audio_latency[interlace]);
} else {
tmp = REG_SET_FIELD(0,
AZALIA_F0_CODEC_PIN_CONTROL_RESPONSE_LIPSYNC,
VIDEO_LIPSYNC, 0);
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index c14b70350a51..896f0416b69f 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -1283,11 +1283,11 @@ static void dce_v11_0_audio_write_latency_fields(struct
drm_encoder *encoder,
if (mode->flags & DRM_MODE_FLAG_INTERLACE)
interlace = 1;
- if (connector->latency_present[interlace]) {
+ if (connector->display_info.latency_present[interlace]) {
tmp = REG_SET_FIELD(0,
AZALIA_F0_CODEC_PIN_CONTROL_RESPONSE_LIPSYNC,
- VIDEO_LIPSYNC,
connector->video_latency[interlace]);
+ VIDEO_LIPSYNC,
connector->display_info.video_latency[interlace]);
tmp = REG_SET_FIELD(0,
AZALIA_F0_CODEC_PIN_CONTROL_RESPONSE_LIPSYNC,
- AUDIO_LIPSYNC,
connector->audio_latency[interlace]);
+ AUDIO_LIPSYNC,
connector->display_info.audio_latency[interlace]);
} else {
tmp = REG_SET_FIELD(0,
AZALIA_F0_CODEC_PIN_CONTROL_RESPONSE_LIPSYNC,
VIDEO_LIPSYNC, 0);
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index 7f85ba5b726f..4aa797726bca 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
@@ -1158,11 +1158,11 @@ static void dce_v6_0_audio_write_latency_fields(struct
drm_encoder *encoder,
if (mode->flags & DRM_MODE_FLAG_INTERLACE)
interlac
Re: [Freedreno] [PATCH v2] drm/probe_helper: sort out poll_running vs poll_enabled
sable - disable output polling
> * @dev: drm_device
> @@ -848,7 +854,12 @@ static void drm_kms_helper_poll_disable_fini(struct
> drm_device *dev, bool fini)
> */
> void drm_kms_helper_poll_disable(struct drm_device *dev)
> {
> - drm_kms_helper_poll_disable_fini(dev, false);
> + if (dev->mode_config.poll_running)
> + drm_kms_helper_disable_hpd(dev);
> +
> + cancel_delayed_work_sync(>mode_config.output_poll_work);
> +
> + dev->mode_config.poll_running = false;
> }
> EXPORT_SYMBOL(drm_kms_helper_poll_disable);
>
> @@ -886,7 +897,12 @@ EXPORT_SYMBOL(drm_kms_helper_poll_init);
> */
> void drm_kms_helper_poll_fini(struct drm_device *dev)
> {
> - drm_kms_helper_poll_disable_fini(dev, true);
> + if (!dev->mode_config.poll_enabled)
> + return;
> +
> + drm_kms_helper_poll_disable(dev);
> +
> + dev->mode_config.poll_enabled = false;
> }
> EXPORT_SYMBOL(drm_kms_helper_poll_fini);
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH] drm/probe_helper: sort out poll_running vs poll_enabled
t;mode_config.poll_running = drm_kms_helper_poll;
>> > + drm_kms_helper_poll_enable(dev);
>> >
>> > if (connector->status == connector_status_disconnected) {
>> > DRM_DEBUG_KMS("[CONNECTOR:%d:%s] disconnected\n",
>> > @@ -710,8 +736,12 @@ static void output_poll_execute(struct work_struct
>> > *work)
>> > changed = dev->mode_config.delayed_event;
>> > dev->mode_config.delayed_event = false;
>> >
>> > - if (!drm_kms_helper_poll)
>> > + if (!drm_kms_helper_poll &&
>> > + dev->mode_config.poll_running) {
>> > + drm_kms_helper_disable_hpd(dev);
>> > + dev->mode_config.poll_running = false;
>> > goto out;
>> > + }
>> >
>> > if (!mutex_trylock(>mode_config.mutex)) {
>> > repoll = true;
>> > @@ -808,30 +838,6 @@ bool drm_kms_helper_is_poll_worker(void)
>> > }
>> > EXPORT_SYMBOL(drm_kms_helper_is_poll_worker);
>> >
>> > -static void drm_kms_helper_poll_disable_fini(struct drm_device *dev, bool
>> > fini)
>> > -{
>> > - struct drm_connector *connector;
>> > - struct drm_connector_list_iter conn_iter;
>> > -
>> > - if (!dev->mode_config.poll_enabled)
>> > - return;
>> > -
>> > - if (fini)
>> > - dev->mode_config.poll_enabled = false;
>> > -
>> > - drm_connector_list_iter_begin(dev, _iter);
>> > - drm_for_each_connector_iter(connector, _iter) {
>> > - const struct drm_connector_helper_funcs *funcs =
>> > - connector->helper_private;
>> > -
>> > - if (funcs && funcs->disable_hpd)
>> > - funcs->disable_hpd(connector);
>> > - }
>> > - drm_connector_list_iter_end(_iter);
>> > -
>> > - cancel_delayed_work_sync(>mode_config.output_poll_work);
>> > -}
>> > -
>> > /**
>> > * drm_kms_helper_poll_disable - disable output polling
>> > * @dev: drm_device
>> > @@ -848,7 +854,12 @@ static void drm_kms_helper_poll_disable_fini(struct
>> > drm_device *dev, bool fini)
>> > */
>> > void drm_kms_helper_poll_disable(struct drm_device *dev)
>> > {
>> > - drm_kms_helper_poll_disable_fini(dev, false);
>> > + if (dev->mode_config.poll_running)
>> > + drm_kms_helper_disable_hpd(dev);
>> > +
>> > + cancel_delayed_work_sync(>mode_config.output_poll_work);
>> > +
>> > + dev->mode_config.poll_running = false;
>> > }
>> > EXPORT_SYMBOL(drm_kms_helper_poll_disable);
>> >
>> > @@ -886,7 +897,12 @@ EXPORT_SYMBOL(drm_kms_helper_poll_init);
>> > */
>> > void drm_kms_helper_poll_fini(struct drm_device *dev)
>> > {
>> > - drm_kms_helper_poll_disable_fini(dev, true);
>> > + if (!dev->mode_config.poll_enabled)
>> > + return;
>> > +
>> > + drm_kms_helper_poll_disable(dev);
>> > +
>> > + dev->mode_config.poll_enabled = false;
>> > }
>> > EXPORT_SYMBOL(drm_kms_helper_poll_fini);
>> >
>> > --
>> > 2.39.0
>> >
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [RFC PATCH] drm/edid: Make 144 Hz not preferred on Sharp LQ140M1JW46
On Wed, 17 Aug 2022, Doug Anderson wrote: > Hi, > > On Sun, Aug 14, 2022 at 11:46 PM Maxime Ripard wrote: >> >> On Fri, Jul 29, 2022 at 12:57:40PM -0700, Doug Anderson wrote: >> > Hi, >> > >> > On Fri, Jul 29, 2022 at 9:41 AM Maxime Ripard wrote: >> > > You raise some good points, but most of the discussion around that patch >> > > were mostly around performances, power consumption and so on. >> > > >> > > This is very much a policy decision, and if there is some panel where >> > > the EDID reports 60Hz but is broken, then that panel should be the >> > > exception to the policy >> > > >> > > But doing it for a single panel is just odd >> > >> > OK, fair enough. I'll abandon this patch at least as far as mainline >> > is concerned, then. >> >> That wasn't really my point though :) >> >> If you think that this change is needed, then we should totally discuss >> it and I'm not opposed to it. >> >> What I don't really like about this patch is that it's about a single >> panel: if we're doing it we should do it for all the panels. >> >> Where we do it can also be discussed, but we should remain consistent >> there. > > I was never massively confident about it, which is why I added the > "RFC" tag to begin with. ;-) In general I suspect that either change > will make people upset. In other words, if we programmatically always > try to put the "high refresh rate" first for all displays then people > will be upset and if we programmatically always try to put the "60 Hz > rate" first then people will be upset. Unless someone wants to stand > up and say that one side or the other is wrong, I think we're going to > simply leave this up to the whim of individual panels. Someone could > stand up and demand that it go one way or the other, but I certainly > don't have that clout. > > The spec, as far as I can tell, says that it's up to the panel vendor > to use whatever means they want to decide on the "preferred" refresh > rate. Thus, as far as the spec is concerned this decision is made on > an individual panel basis. ;-) This was really the justification for > why my patch was just on one panel. > > In any case, as I said I'm OK w/ dropping this. We'll find other ways > to work around the issue. FWIW, if the EDID reported preferred mode works, I also think that's what we should prefer. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH v4] drm/probe-helper: Default to 640x480 if no EDID on DP
On Wed, 01 Jun 2022, Douglas Anderson wrote:
> If we're unable to read the EDID for a display because it's corrupt /
> bogus / invalid then we'll add a set of standard modes for the
> display. Since we have no true information about the connected
> display, these modes are essentially guesses but better than nothing.
> At the moment, none of the modes returned is marked as preferred, but
> the modes are sorted such that the higher resolution modes are listed
> first.
>
> When userspace sees these modes presented by the kernel it needs to
> figure out which one to pick. At least one userspace, ChromeOS [1]
> seems to use the rules (which seem pretty reasonable):
> 1. Try to pick the first mode marked as preferred.
> 2. Try to pick the mode which matches the first detailed timing
>descriptor in the EDID.
> 3. If no modes were marked as preferred then pick the first mode.
>
> Unfortunately, userspace's rules combined with what the kernel is
> doing causes us to fail section 4.2.2.6 (EDID Corruption Detection) of
> the DP 1.4a Link CTS. That test case says that, while it's OK to allow
> some implementation-specific fall-back modes if the EDID is bad that
> userspace should _default_ to 640x480.
>
> Let's fix this by marking 640x480 as default for DP in the no-EDID
> case.
>
> NOTES:
> - In the discussion around v3 of this patch [2] there was talk about
> solving this in userspace and I even implemented a patch that would
> have solved this for ChromeOS, but then the discussion turned back
> to solving this in the kernel.
> - Also in the discussion of v3 [2] it was requested to limit this
> 83;40900;0c change to just DP since folks were worried that it would break
> some
> subtle corner case on VGA or HDMI.
>
> [1]
> https://source.chromium.org/chromium/chromium/src/+/a051f741d0a15caff2251301efe081c30e0f4a96:ui/ozone/platform/drm/common/drm_util.cc;l=488
> [2]
> https://lore.kernel.org/r/20220513130533.v3.1.I31ec454f8d4ffce51a7708a8092f8a6f9c929092@changeid
>
> Signed-off-by: Douglas Anderson
> Reviewed-by: Abhinav Kumar
> ---
> I put Abhinav's Reviewed-by tag from v2 here since this is nearly the
> same as v2. Hope this is OK.
>
> Changes in v4:
> - Code is back to v2, but limit to just DP.
> - Beefed up the commit message.
>
> Changes in v3:
> - Don't set preferred, just disable the sort.
>
> Changes in v2:
> - Don't modify drm_add_modes_noedid() 'cause that'll break others
> - Set 640x480 as preferred in drm_helper_probe_single_connector_modes()
>
> drivers/gpu/drm/drm_probe_helper.c | 11 ++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> b/drivers/gpu/drm/drm_probe_helper.c
> index 425f56280d51..75a71649b64d 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -569,8 +569,17 @@ int drm_helper_probe_single_connector_modes(struct
> drm_connector *connector,
> count = drm_add_override_edid_modes(connector);
>
> if (count == 0 && (connector->status == connector_status_connected ||
> -connector->status == connector_status_unknown))
> +connector->status == connector_status_unknown)) {
> count = drm_add_modes_noedid(connector, 1024, 768);
> +
> + /*
> + * Section 4.2.2.6 (EDID Corruption Detection) of the DP 1.4a
> + * Link CTS specifies that 640x480 (the official "failsafe"
> + * mode) needs to be the default if there's no EDID.
> + */
> + if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort)
If we're doing this primarily to appease the CTS, this is fine.
If we think this is a functional improvement for regular use, I suppose
we should consider doing this also for DRM_MODE_CONNECTOR_eDP. Which is
irrelevant for the CTS.
Either way,
Acked-by: Jani Nikula
> + drm_set_preferred_mode(connector, 640, 480);
> + }
> count += drm_helper_probe_add_cmdline_mode(connector);
> if (count != 0) {
> ret = __drm_helper_update_and_validate(connector, maxX, maxY,
> );
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [RFC] drm/msm: Add initial ci/ subdirectory
On Tue, 10 May 2022, Tomeu Vizoso wrote: > And use it to store expectations about what the drm/msm driver is > supposed to pass in the IGT test suite. > > Also include a configuration file that points to the out-of-tree CI > scripts. > > By storing the test expectations along the code we can make sure both > stay in sync with each other, and so we can know when a code change > breaks those expectations. > > This will allow all contributors to drm/msm to reuse the infrastructure > already in gitlab.freedesktop.org to test the driver on several > generations of the hardware. Cc: Petri and Arek The tests and subtests and their names are inevitably going to change over time. I don't think there are any guarantees that they're stable. How are you going to handle such changes with the test names and results committed to the kernel tree? BR, Jani. > > Signed-off-by: Tomeu Vizoso > --- > Documentation/gpu/msm_automated_testing.rst | 70 > drivers/gpu/drm/msm/ci/gitlab-ci.yml| 11 + > drivers/gpu/drm/msm/ci/msm.testlist | 429 > drivers/gpu/drm/msm/ci/msm_a360_results.txt | 421 +++ > drivers/gpu/drm/msm/ci/msm_a530_results.txt | 421 +++ > drivers/gpu/drm/msm/ci/msm_a618_results.txt | 422 +++ > drivers/gpu/drm/msm/ci/msm_a630_results.txt | 422 +++ > 7 files changed, 2196 insertions(+) > create mode 100644 Documentation/gpu/msm_automated_testing.rst > create mode 100644 drivers/gpu/drm/msm/ci/gitlab-ci.yml > create mode 100644 drivers/gpu/drm/msm/ci/msm.testlist > create mode 100644 drivers/gpu/drm/msm/ci/msm_a360_results.txt > create mode 100644 drivers/gpu/drm/msm/ci/msm_a530_results.txt > create mode 100644 drivers/gpu/drm/msm/ci/msm_a618_results.txt > create mode 100644 drivers/gpu/drm/msm/ci/msm_a630_results.txt > > diff --git a/Documentation/gpu/msm_automated_testing.rst > b/Documentation/gpu/msm_automated_testing.rst > new file mode 100644 > index ..fe59474569c2 > --- /dev/null > +++ b/Documentation/gpu/msm_automated_testing.rst > @@ -0,0 +1,70 @@ > +.. SPDX-License-Identifier: GPL-2.0+ > + > += > +Automated testing of MSM DRM driver > += > + > + > +Introduction > + > + > +Making sure that changes to a driver don't introduce regressions can be very > time consuming when lots of different hardware configurations need to be > tested. Moreover, it isn't practical for each person interested in this > testing to have to acquire and maintain what can be a considerable amount of > hardware. > + > +Also, it is desirable for developers to check for regressions in their code > by themselves, instead of relying on the maintainer finding them and then > reporting back. > + > +There are facilities in gitlab.freedesktop.org to automatically test Mesa > that can be used as well for testing DRM drivers such as drm/msm. This > document explains how people interested in testing the drm/msm driver can use > this shared infrastructure to save quite some time and effort. > + > + > +Relevant files > +== > + > +drivers/gpu/drm/msm/ci/gitlab-ci.yml > + > + > +Specifies the specific version of the scripts to be used. GitLab CI will use > the values defined in this file to fetch the right scripts. > + > + > +drivers/gpu/drm/msm/ci/msm.testlist > +--- > + > +Specifies the tests that the current code is expected to be able to reliably > run. These tests are expected to not hang the DUT (device under testing) when > running on the revision they belong to, and to give consistent results. > + > + > +drivers/gpu/drm/msm/ci/msm_*_results.txt > + > + > +Specifies the expected results of running this specific kernel revision on a > given hardware configuration. > + > + > +How to enable automated testing on your tree > + > + > +1. Create a Linux tree in https://gitlab.freedesktop.org/ if you don't have > one yet > + > +2. In your kernel repo's configuration (eg. > https://gitlab.freedesktop.org/tomeu/linux/-/settings/ci_cd), change the > CI/CD configuration file from .gitlab-ci.yml to > drivers/gpu/drm/msm/ci/gitlab-ci.yml. > + > +3. Next time you push to this repository, you will see a CI pipeline being > created (eg. https://gitlab.freedesktop.org/tomeu/linux/-/pipelines) > + > +4. The various jobs will be run and when the pipeline is finished, all jobs > should be green unless a regression has been found. > + > + > +How to update test expectations > +=== > + > +If your changes to the code fix any tests, you will have to update one or > more of the files in drivers/gpu/drm/msm/ci/msm_*_results.txt, for each of > the test platforms affected by the change. > + > +If you have run a pipeline and it failed
Re: [Freedreno] [PATCH v3 0/3] Add connector_type to debug info to differentiate between eDP and DP
On Wed, 02 Feb 2022, Kuogee Hsieh wrote: > 1) Add connector_type to debug info to differentiate between eDP and DP > 2) add more debug info to cover dp Phy > 3) repalce DRM_DEBUG_DP with drm_debug_dp In the future, please include the drm/msm/dp prefix also in the cover letter. Thanks. BR, Jani. > > Kuogee Hsieh (3): > drm/msm/dp: add connector type to enhance debug messages > drm/msm/dp: enhance debug info related to dp phy > drm/msm/dp: replace DRM_DEBUG_DP marco with drm_dbg_dp > > drivers/gpu/drm/msm/dp/dp_audio.c | 49 +-- > drivers/gpu/drm/msm/dp/dp_catalog.c | 34 ++- > drivers/gpu/drm/msm/dp/dp_ctrl.c| 116 > +++- > drivers/gpu/drm/msm/dp/dp_display.c | 103 ++-- > drivers/gpu/drm/msm/dp/dp_drm.c | 4 +- > drivers/gpu/drm/msm/dp/dp_link.c| 99 +- > drivers/gpu/drm/msm/dp/dp_panel.c | 43 +++-- > drivers/gpu/drm/msm/dp/dp_parser.c | 2 +- > drivers/gpu/drm/msm/dp/dp_power.c | 20 --- > 9 files changed, 283 insertions(+), 187 deletions(-) -- Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [RFC PATCH] drm: allow passing a real encoder object for wb connector
On Thu, 20 Jan 2022, Abhinav Kumar wrote:
> Instead of creating an internal encoder for the writeback
> connector to satisfy DRM requirements, allow the clients
> to pass a real encoder to it by changing the drm_writeback's
> encoder to a pointer.
>
> If a real encoder is not passed, drm_writeback_connector_init
> will internally allocate one.
>
> This will help the clients to manage the real encoder states
> better as they will allocate and maintain the encoder.
See also the thread starting at [1], and please try to coordinate.
I don't know what the end result should be like, I'm just saying please
collaborate instead of racing to get one set of changes in.
BR,
Jani.
[1]
https://patchwork.freedesktop.org/patch/msgid/2022001801.28310-1-suraj.kand...@intel.com
>
> Signed-off-by: Abhinav Kumar
> ---
> drivers/gpu/drm/drm_writeback.c | 11 +++
> include/drm/drm_writeback.h | 2 +-
> 2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index dccf4504..fdb7381 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -189,8 +189,11 @@ int drm_writeback_connector_init(struct drm_device *dev,
> if (IS_ERR(blob))
> return PTR_ERR(blob);
>
> - drm_encoder_helper_add(_connector->encoder, enc_helper_funcs);
> - ret = drm_encoder_init(dev, _connector->encoder,
> + /* allocate the internal drm encoder if a real one wasnt passed */
> + if (!wb_connector->encoder)
> + wb_connector->encoder = devm_kzalloc(dev->dev, sizeof(struct
> drm_encoder), GFP_KERNEL);
> + drm_encoder_helper_add(wb_connector->encoder, enc_helper_funcs);
> + ret = drm_encoder_init(dev, wb_connector->encoder,
> _writeback_encoder_funcs,
> DRM_MODE_ENCODER_VIRTUAL, NULL);
> if (ret)
> @@ -204,7 +207,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> goto connector_fail;
>
> ret = drm_connector_attach_encoder(connector,
> - _connector->encoder);
> + wb_connector->encoder);
> if (ret)
> goto attach_fail;
>
> @@ -233,7 +236,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> attach_fail:
> drm_connector_cleanup(connector);
> connector_fail:
> - drm_encoder_cleanup(_connector->encoder);
> + drm_encoder_cleanup(wb_connector->encoder);
> fail:
> drm_property_blob_put(blob);
> return ret;
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index 9697d27..f0d8147 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -31,7 +31,7 @@ struct drm_writeback_connector {
>* by passing the @enc_funcs parameter to drm_writeback_connector_init()
>* function.
>*/
> - struct drm_encoder encoder;
> + struct drm_encoder *encoder;
>
> /**
>* @pixel_formats_blob_ptr:
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH v5 03/32] component: Move struct aggregate_device out to header file
On Fri, 07 Jan 2022, Stephen Boyd wrote:
> Quoting Jani Nikula (2022-01-07 05:07:59)
>> On Thu, 06 Jan 2022, Stephen Boyd wrote:
>> > This allows aggregate driver writers to use the device passed to their
>> > probe/remove/shutdown functions properly instead of treating it as an
>> > opaque pointer.
>>
>> You say it like having opaque pointers with interfaces instead of
>> exposed data is a bad thing.
>
> I didn't intend to convey that message at all and in fact I didn't write
> that opaque pointers are a bad thing.
>
>>
>> Data is not an interface. IMO if you can get by with keeping the types
>> private, go for it. Unless I'm missing something, you only need the
>> parent dev pointer. Maybe add a helper function for it?
>
> Sure I'll add a function for that.
>
>>
>> It's trivial to expose the guts like this, but it's usually a lot of
>> hard work to go the other way. Look at the dependencies of component.h
>> now. To keep it self-contained, i.e. buildable without implicit
>> dependencies, you'd need to add #include , which goes on to
>> include the world. Then have a look at [1].
>>
>> Please at least let's not do this lightly.
>>
>
> Got it. Thanks! How does this look?
Thanks, I think this is better.
BR,
Jani.
>
> ---8<---
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index cd50137753b4..e8f09945c261 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -56,6 +56,27 @@ struct component_match {
> struct component_match_array *compare;
> };
>
> +struct aggregate_device {
> + const struct component_master_ops *ops;
> + struct device *parent;
> + struct device dev;
> + struct component_match *match;
> + struct aggregate_driver *adrv;
> +
> + int id;
> +};
> +
> +static inline struct aggregate_device *to_aggregate_device(struct device *d)
> +{
> + return container_of(d, struct aggregate_device, dev);
> +}
> +
> +struct device *aggregate_device_parent(struct aggregate_device *adev)
> +{
> + return adev->parent;
> +}
> +EXPORT_SYMBOL_GPL(aggregate_device_parent);
> +
> struct component {
> struct list_head node;
> struct aggregate_device *adev;
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> index 0463386a6ed2..5fa868cf9825 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> @@ -27,7 +27,7 @@ struct komeda_dev *dev_to_mdev(struct device *dev)
>
> static void komeda_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct komeda_drv *mdrv = dev_get_drvdata(dev);
>
> if (!mdrv)
> @@ -48,7 +48,7 @@ static void komeda_unbind(struct aggregate_device *adev)
>
> static int komeda_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct komeda_drv *mdrv;
> int err;
>
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
> index 5c03eb98d814..e3ed925797d5 100644
> --- a/drivers/gpu/drm/arm/hdlcd_drv.c
> +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
> @@ -272,7 +272,7 @@ static const struct drm_driver hdlcd_driver = {
>
> static int hdlcd_drm_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm;
> struct hdlcd_drm_private *hdlcd;
> int ret;
> @@ -347,7 +347,7 @@ static int hdlcd_drm_bind(struct aggregate_device *adev)
>
> static void hdlcd_drm_unbind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct drm_device *drm = dev_get_drvdata(dev);
> struct hdlcd_drm_private *hdlcd = drm->dev_private;
>
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c
> b/drivers/gpu/drm/arm/malidp_drv.c
> index e6ee4d1e3bb8..7b946b962b22 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -704,7 +704,7 @@ static int malidp_runtime_pm_resume(struct device *dev)
>
> static int malidp_bind(struct aggregate_device *adev)
> {
> - struct device *dev = adev->parent;
> + struct device *dev = aggregate_device_parent(adev);
> struct resource *res;
> struct drm_device *drm;
> struct malidp_drm *malidp;
> @@ -897,7 +8
Re: [Freedreno] [PATCH v5 03/32] component: Move struct aggregate_device out to header file
On Thu, 06 Jan 2022, Stephen Boyd wrote:
> This allows aggregate driver writers to use the device passed to their
> probe/remove/shutdown functions properly instead of treating it as an
> opaque pointer.
You say it like having opaque pointers with interfaces instead of
exposed data is a bad thing.
Data is not an interface. IMO if you can get by with keeping the types
private, go for it. Unless I'm missing something, you only need the
parent dev pointer. Maybe add a helper function for it?
It's trivial to expose the guts like this, but it's usually a lot of
hard work to go the other way. Look at the dependencies of component.h
now. To keep it self-contained, i.e. buildable without implicit
dependencies, you'd need to add #include , which goes on to
include the world. Then have a look at [1].
Please at least let's not do this lightly.
BR,
Jani.
[1] https://lwn.net/ml/linux-kernel/ydifz+lmewets...@gmail.com/
>
> Cc: Daniel Vetter
> Cc: Greg Kroah-Hartman
> Cc: Laurent Pinchart
> Cc: "Rafael J. Wysocki"
> Cc: Rob Clark
> Cc: Russell King
> Cc: Saravana Kannan
> Signed-off-by: Stephen Boyd
> ---
> drivers/base/component.c | 15 ---
> include/linux/component.h | 19 ---
> 2 files changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index eec82caeae5e..dc38a8939ae6 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -56,21 +56,6 @@ struct component_match {
> struct component_match_array *compare;
> };
>
> -struct aggregate_device {
> - const struct component_master_ops *ops;
> - struct device *parent;
> - struct device dev;
> - struct component_match *match;
> - struct aggregate_driver *adrv;
> -
> - int id;
> -};
> -
> -static inline struct aggregate_device *to_aggregate_device(struct device *d)
> -{
> - return container_of(d, struct aggregate_device, dev);
> -}
> -
> struct component {
> struct list_head node;
> struct aggregate_device *adev;
> diff --git a/include/linux/component.h b/include/linux/component.h
> index 95d1b23ede8a..e99cf8e910f0 100644
> --- a/include/linux/component.h
> +++ b/include/linux/component.h
> @@ -5,6 +5,8 @@
> #include
> #include
>
> +struct component_match;
> +
> /**
> * struct component_ops - callbacks for component drivers
> *
> @@ -39,8 +41,6 @@ void component_del(struct device *, const struct
> component_ops *);
> int component_bind_all(struct device *master, void *master_data);
> void component_unbind_all(struct device *master, void *master_data);
>
> -struct aggregate_device;
> -
> /**
> * struct component_master_ops - callback for the aggregate driver
> *
> @@ -80,7 +80,20 @@ struct component_master_ops {
> void (*unbind)(struct device *master);
> };
>
> -struct component_match;
> +struct aggregate_device {
> + const struct component_master_ops *ops;
> + struct device *parent;
> + struct device dev;
> + struct component_match *match;
> + struct aggregate_driver *adrv;
> +
> + int id;
> +};
> +
> +static inline struct aggregate_device *to_aggregate_device(struct device *d)
> +{
> + return container_of(d, struct aggregate_device, dev);
> +}
>
> /**
> * struct aggregate_driver - Aggregate driver (made up of other drivers)
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH v2 3/5] drm/dp: Move DisplayPort helpers into separate helper module
On Wed, 15 Dec 2021, Thomas Zimmermann wrote: > * move DP helper code into dp/ (Jani) I suggested adding the subdirectory, but I'm going to bikeshed the name, which I didn't suggest. $ find drivers/gpu/drm -mindepth 1 -maxdepth 1 -type d | wc -l 68 Assuming we move more of the drm modules to subdirectories, how are they going to stand out from drivers? I suggested drm_dp, which I understand results in tautology, but hey, all the filenames under drm/ also have drm_*.[ch]. And I find that very useful for git greps and other code archeology. With just the dp name, you'd have to know and list all the drm subdirectories when looking up stuff that's part of drm but not drivers. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH 0/3] drm/dp: Move DisplayPort helpers into own module
On Mon, 13 Dec 2021, Thomas Zimmermann wrote: > Hi > > Am 13.12.21 um 14:34 schrieb Jani Nikula: >> On Mon, 13 Dec 2021, Thomas Zimmermann wrote: >>> Split-off DisplayPort functions from KMS helper library and move them >>> into their own module. Reduces the size of drm_kms_helper.ko by ~50%. >>> >>> This patchset is part of an on-going effort to reduce the minimum >>> binary size of the DRM core and helpers. It's helpful for systems with >>> early-boot DRM graphics, which requires DRM to be linked into the >>> kernel image. >> >> Would it be time to add a subdirectory for each non-driver, non-core drm >> module? We've touched this topic before. I find it increasingly hard to >> remember which files are part of helpers. This would also help with the >> arbitrary drm_dp_helper_mod.c naming. >> >> Perhaps drivers/gpu/drm/drm_dp/? > > It's probably worth it, but I'd prefer a separate patchset and > discussion over this. It affects several modules. I guess the only thing here that we need to get right from the start is the new module name, everything else is relatively easy to change later. drm_dp_helper.ko seems fine by me. Note that this will also affect the drm_kms_helper.ko module parameters dp_aux_i2c_speed_khz, dp_aux_i2c_transfer_size and drm_dp_cec_unregister_delay, which will move to drm_dp_helper.ko. See the monstrosity near the top of drm_kms_helper_common.c I had to add for backward compatibility when I moved drm_edid_load.c from drm_kms_helper.ko to drm.ko. That was perhaps different, as these seem more like debug knobs, but at a minimum this needs to be mentioned in the commit message, and certainly needs acks from Dave and/or Daniel. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH 0/3] drm/dp: Move DisplayPort helpers into own module
On Mon, 13 Dec 2021, Thomas Zimmermann wrote:
> Split-off DisplayPort functions from KMS helper library and move them
> into their own module. Reduces the size of drm_kms_helper.ko by ~50%.
>
> This patchset is part of an on-going effort to reduce the minimum
> binary size of the DRM core and helpers. It's helpful for systems with
> early-boot DRM graphics, which requires DRM to be linked into the
> kernel image.
Would it be time to add a subdirectory for each non-driver, non-core drm
module? We've touched this topic before. I find it increasingly hard to
remember which files are part of helpers. This would also help with the
arbitrary drm_dp_helper_mod.c naming.
Perhaps drivers/gpu/drm/drm_dp/?
BR,
Jani.
>
> Thomas Zimmermann (3):
> drm/dp_mst: Remove trailing whitespace.
> drm/dp: Move DP declarations into separate header file
> drm/dp: Move DisplayPort helpers into separate helper module
>
> drivers/gpu/drm/Kconfig | 8 ++
> drivers/gpu/drm/Makefile | 14 ++
> drivers/gpu/drm/bridge/Kconfig| 4 +++
> drivers/gpu/drm/bridge/analogix/Kconfig | 2 ++
> drivers/gpu/drm/bridge/cadence/Kconfig| 1 +
> drivers/gpu/drm/drm_crtc_helper_internal.h| 27 --
> drivers/gpu/drm/{drm_dp_helper.c => drm_dp.c} | 2 +-
> drivers/gpu/drm/drm_dp_aux_dev.c | 2 +-
> drivers/gpu/drm/drm_dp_helper_internal.h | 28 +++
> drivers/gpu/drm/drm_dp_helper_mod.c | 22 +++
> drivers/gpu/drm/drm_dp_mst_topology.c | 4 +--
> drivers/gpu/drm/drm_kms_helper_common.c | 14 --
> drivers/gpu/drm/i915/Kconfig | 1 +
> drivers/gpu/drm/msm/Kconfig | 1 +
> drivers/gpu/drm/nouveau/Kconfig | 1 +
> drivers/gpu/drm/rockchip/Kconfig | 1 +
> drivers/gpu/drm/tegra/Kconfig | 1 +
> drivers/gpu/drm/xlnx/Kconfig | 1 +
> 18 files changed, 83 insertions(+), 51 deletions(-)
> rename drivers/gpu/drm/{drm_dp_helper.c => drm_dp.c} (99%)
> create mode 100644 drivers/gpu/drm/drm_dp_helper_internal.h
> create mode 100644 drivers/gpu/drm/drm_dp_helper_mod.c
>
>
> base-commit: 3f422828221d9ceefcddef0be33561b1646a1cbe
> prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
> prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
> --
> 2.34.1
>
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [Intel-gfx] [PATCH 2/3] drm/dp: Move DP declarations into separate header file
On Mon, 13 Dec 2021, Thomas Zimmermann wrote:
> Split the DP declarations from other helpers before moving the
> DP functions into a separate module.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_crtc_helper_internal.h | 27 -
> drivers/gpu/drm/drm_dp_aux_dev.c | 2 +-
> drivers/gpu/drm/drm_dp_helper.c| 2 +-
> drivers/gpu/drm/drm_dp_helper_internal.h | 28 ++
> drivers/gpu/drm/drm_dp_mst_topology.c | 2 +-
> drivers/gpu/drm/drm_kms_helper_common.c| 1 +
> 6 files changed, 32 insertions(+), 30 deletions(-)
> create mode 100644 drivers/gpu/drm/drm_dp_helper_internal.h
>
> diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h
> b/drivers/gpu/drm/drm_crtc_helper_internal.h
> index 61e09f8a8d0f..28e04e750130 100644
> --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
> @@ -28,36 +28,9 @@
>
> #include
> #include
> -#include
> #include
> #include
>
> -/* drm_dp_aux_dev.c */
> -#ifdef CONFIG_DRM_DP_AUX_CHARDEV
> -int drm_dp_aux_dev_init(void);
> -void drm_dp_aux_dev_exit(void);
> -int drm_dp_aux_register_devnode(struct drm_dp_aux *aux);
> -void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux);
> -#else
> -static inline int drm_dp_aux_dev_init(void)
> -{
> - return 0;
> -}
> -
> -static inline void drm_dp_aux_dev_exit(void)
> -{
> -}
> -
> -static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
> -{
> - return 0;
> -}
> -
> -static inline void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux)
> -{
> -}
> -#endif
> -
> /* drm_probe_helper.c */
> enum drm_mode_status drm_crtc_mode_valid(struct drm_crtc *crtc,
>const struct drm_display_mode *mode);
> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
> b/drivers/gpu/drm/drm_dp_aux_dev.c
> index 06b374cae956..0618dfe16660 100644
> --- a/drivers/gpu/drm/drm_dp_aux_dev.c
> +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> @@ -40,7 +40,7 @@
> #include
> #include
>
> -#include "drm_crtc_helper_internal.h"
> +#include "drm_dp_helper_internal.h"
>
> struct drm_dp_aux_dev {
> unsigned index;
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 23f9073bc473..e995a0262ed7 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -35,7 +35,7 @@
> #include
> #include
>
> -#include "drm_crtc_helper_internal.h"
> +#include "drm_dp_helper_internal.h"
>
> struct dp_aux_backlight {
> struct backlight_device *base;
> diff --git a/drivers/gpu/drm/drm_dp_helper_internal.h
> b/drivers/gpu/drm/drm_dp_helper_internal.h
> new file mode 100644
> index ..5c9f8bb0c99a
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_dp_helper_internal.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +#include
Please don't include other headers if you can avoid them by using
forward declarations.
BR,
Jani.
> +
> +#ifdef CONFIG_DRM_DP_AUX_CHARDEV
> +int drm_dp_aux_dev_init(void);
> +void drm_dp_aux_dev_exit(void);
> +int drm_dp_aux_register_devnode(struct drm_dp_aux *aux);
> +void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux);
> +#else
> +static inline int drm_dp_aux_dev_init(void)
> +{
> + return 0;
> +}
> +
> +static inline void drm_dp_aux_dev_exit(void)
> +{
> +}
> +
> +static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
> +{
> + return 0;
> +}
> +
> +static inline void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux)
> +{
> +}
> +#endif
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 7f0ff96261cf..9f7b0b606924 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -45,7 +45,7 @@
> #include
> #include
>
> -#include "drm_crtc_helper_internal.h"
> +#include "drm_dp_helper_internal.h"
> #include "drm_dp_mst_topology_internal.h"
>
> /**
> diff --git a/drivers/gpu/drm/drm_kms_helper_common.c
> b/drivers/gpu/drm/drm_kms_helper_common.c
> index 47e92400548d..88260d26409c 100644
> --- a/drivers/gpu/drm/drm_kms_helper_common.c
> +++ b/drivers/gpu/drm/drm_kms_helper_common.c
> @@ -29,6 +29,7 @@
>
> #include
>
> +#include "drm_dp_helper_internal.h"
> #include "drm_crtc_helper_internal.h"
>
> MODULE_AUTHOR("David Airlie, Jesse Barnes");
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH bpf] treewide: add missing includes masked by cgroup -> bpf dependency
On Fri, 19 Nov 2021, Jakub Kicinski wrote: > cgroup.h (therefore swap.h, therefore half of the universe) > includes bpf.h which in turn includes module.h and slab.h. > Since we're about to get rid of that dependency we need > to clean things up. > > Signed-off-by: Jakub Kicinski > --- > CC: ax...@kernel.dk > CC: maarten.lankho...@linux.intel.com > CC: mrip...@kernel.org > CC: tzimmerm...@suse.de > CC: airl...@linux.ie > CC: dan...@ffwll.ch > CC: jani.nik...@linux.intel.com > CC: joonas.lahti...@linux.intel.com > CC: rodrigo.v...@intel.com > CC: yuq...@gmail.com > CC: robdcl...@gmail.com > CC: s...@poorly.run > CC: christian.koe...@amd.com > CC: ray.hu...@amd.com > CC: sgout...@marvell.com > CC: gak...@marvell.com > CC: sbha...@marvell.com > CC: hke...@marvell.com > CC: jingooh...@gmail.com > CC: lorenzo.pieral...@arm.com > CC: r...@kernel.org > CC: k...@linux.com > CC: bhelg...@google.com > CC: krzysztof.kozlow...@canonical.com > CC: m...@kernel.org > CC: paw...@cadence.com > CC: peter.c...@kernel.org > CC: rog...@kernel.org > CC: a-govindr...@ti.com > CC: gre...@linuxfoundation.org > CC: a...@kernel.org > CC: dan...@iogearbox.net > CC: and...@kernel.org > CC: ka...@fb.com > CC: songliubrav...@fb.com > CC: y...@fb.com > CC: john.fastab...@gmail.com > CC: kpsi...@kernel.org > CC: s...@kernel.org > CC: a...@linux-foundation.org > CC: thomas.hellst...@linux.intel.com > CC: matthew.a...@intel.com > CC: colin.k...@intel.com > CC: ge...@linux-m68k.org > CC: linux-bl...@vger.kernel.org > CC: dri-de...@lists.freedesktop.org > CC: intel-...@lists.freedesktop.org > CC: l...@lists.freedesktop.org > CC: linux-arm-...@vger.kernel.org > CC: freedreno@lists.freedesktop.org > CC: linux-...@vger.kernel.org > CC: linux-arm-ker...@lists.infradead.org > CC: linux-samsung-...@vger.kernel.org > CC: linux-...@vger.kernel.org > CC: b...@vger.kernel.org > CC: linux...@kvack.org > > Well, let's see if this makes it thru email servers... > --- > block/fops.c | 1 + > drivers/gpu/drm/drm_gem_shmem_helper.c | 1 + > drivers/gpu/drm/i915/gt/intel_gtt.c | 1 + > drivers/gpu/drm/i915/i915_request.c | 1 + For the i915 parts, Acked-by: Jani Nikula -- Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [Intel-gfx] [PATCH 15/15] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
On Fri, 15 Oct 2021, Ville Syrjälä wrote:
> On Fri, Oct 15, 2021 at 03:44:48PM +0300, Jani Nikula wrote:
>> On Fri, 15 Oct 2021, Claudio Suarez wrote:
>> > Once EDID is parsed, the monitor HDMI support information is available
>> > through drm_display_info.is_hdmi. Retriving the same information with
>> > drm_detect_hdmi_monitor() is less efficient. Change to
>> > drm_display_info.is_hdmi where possible.
>> >
>> > This is a TODO task in Documentation/gpu/todo.rst
>> >
>> > Signed-off-by: Claudio Suarez
>> > ---
>> > drivers/gpu/drm/i915/display/intel_connector.c | 5 +
>> > drivers/gpu/drm/i915/display/intel_connector.h | 1 +
>> > drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
>> > drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++-
>> > 4 files changed, 9 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_connector.c
>> > b/drivers/gpu/drm/i915/display/intel_connector.c
>> > index 9bed1ccecea0..3346b55df6e1 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_connector.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_connector.c
>> > @@ -213,6 +213,11 @@ int intel_ddc_get_modes(struct drm_connector
>> > *connector,
>> >return ret;
>> > }
>> >
>> > +bool intel_connector_is_hdmi_monitor(struct drm_connector *connector)
>> > +{
>> > + return connector->display_info.is_hdmi;
>> > +}
>> > +
>>
>> A helper like this belongs in drm, not i915. Seems useful in other
>> drivers too.
>
> Not sure it's actually helpful for i915. We end up having to root around
> in the display_info in a lot of places anyway. So a helper for single
> boolean seems a bit out of place perhaps.
*shrug*
Maybe it's just my frustration at the lack of interfaces and poking
around in the depths of nested structs and pointer chasing that's coming
through. You just need to change so many things if you want to later
refactor where "is hdmi" comes from and is stored.
Anyway, if a helper is being added like in this series, I think it
should be one helper in drm, not redundant copies in multiple
drivers. Or we should not have the helper(s) at all. One or the other,
not the worst of both worlds.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH 15/15] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
On Fri, 15 Oct 2021, Claudio Suarez wrote:
> Once EDID is parsed, the monitor HDMI support information is available
> through drm_display_info.is_hdmi. Retriving the same information with
> drm_detect_hdmi_monitor() is less efficient. Change to
> drm_display_info.is_hdmi where possible.
>
> This is a TODO task in Documentation/gpu/todo.rst
>
> Signed-off-by: Claudio Suarez
> ---
> drivers/gpu/drm/i915/display/intel_connector.c | 5 +
> drivers/gpu/drm/i915/display/intel_connector.h | 1 +
> drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
> drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++-
> 4 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_connector.c
> b/drivers/gpu/drm/i915/display/intel_connector.c
> index 9bed1ccecea0..3346b55df6e1 100644
> --- a/drivers/gpu/drm/i915/display/intel_connector.c
> +++ b/drivers/gpu/drm/i915/display/intel_connector.c
> @@ -213,6 +213,11 @@ int intel_ddc_get_modes(struct drm_connector *connector,
> return ret;
> }
>
> +bool intel_connector_is_hdmi_monitor(struct drm_connector *connector)
> +{
> + return connector->display_info.is_hdmi;
> +}
> +
A helper like this belongs in drm, not i915. Seems useful in other
drivers too.
BR,
Jani.
> static const struct drm_prop_enum_list force_audio_names[] = {
> { HDMI_AUDIO_OFF_DVI, "force-dvi" },
> { HDMI_AUDIO_OFF, "off" },
> diff --git a/drivers/gpu/drm/i915/display/intel_connector.h
> b/drivers/gpu/drm/i915/display/intel_connector.h
> index 661a37a3c6d8..ceda6e72ece6 100644
> --- a/drivers/gpu/drm/i915/display/intel_connector.h
> +++ b/drivers/gpu/drm/i915/display/intel_connector.h
> @@ -27,6 +27,7 @@ enum pipe intel_connector_get_pipe(struct intel_connector
> *connector);
> int intel_connector_update_modes(struct drm_connector *connector,
>struct edid *edid);
> int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter
> *adapter);
> +bool intel_connector_is_hdmi_monitor(struct drm_connector *connector);
> void intel_attach_force_audio_property(struct drm_connector *connector);
> void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
> void intel_attach_aspect_ratio_property(struct drm_connector *connector);
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index b04685bb6439..2b1d7c5bebdd 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
> to_intel_connector(connector)->detect_edid = edid;
> if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
> intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
> - intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
> + intel_hdmi->has_hdmi_sink =
> intel_connector_is_hdmi_monitor(connector);
>
> connected = true;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c
> b/drivers/gpu/drm/i915/display/intel_sdvo.c
> index 6cb27599ea03..a32279e4fee8 100644
> --- a/drivers/gpu/drm/i915/display/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
> @@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector
> *connector)
> if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> status = connector_status_connected;
> if (intel_sdvo_connector->is_hdmi) {
> - intel_sdvo->has_hdmi_monitor =
> drm_detect_hdmi_monitor(edid);
> intel_sdvo->has_hdmi_audio =
> drm_detect_monitor_audio(edid);
> + intel_sdvo->has_hdmi_monitor =
> +
> intel_connector_is_hdmi_monitor(connector);
> }
> } else
> status = connector_status_disconnected;
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [Intel-gfx] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Mon, 04 Oct 2021, Ville Syrjälä wrote: > On Sun, Oct 03, 2021 at 12:32:14AM +0200, Fernando Ramos wrote: >> On 21/10/02 09:13AM, Fernando Ramos wrote: >> > >> > Sean, could you revert the whole patch series? I'll have a deeper look >> > into the >> > patch set and come up with a v3 where all these issues will be addressed. >> > >> >> Hi Sean, >> >> I now understand the nature of the issue that caused the problem with i915 >> and >> have proceed to remove the global context structure (which revealed a similar >> issue in the amdgpu driver). >> >> I have prepared a V3 version of the patch set where these issues should >> hopefully be fixed for both the i915 and amdgpu drivers. >> >> In order to prevent causing more disruption, could you tell me what the >> proper >> way to proceed would be? In particular: >> >> 1. Is there any place where I can push my changes so that they are tested >> on a i915 machine? (Some type of automated pool) > > cc:intel-gfx, which it looks like you did, _but_ your patches did > did not even apply against drm-tip so our CI rejected it. There was > a reply to the patches from CI indicating that. And that is one > reason I probably just ignored the whole thing. If it doesn't > even apply/build it's not worth my time to read. > >> >> 2. I can test the amdgpu driver on my machine but, what about all the other >> architectures? What is the standard procedure? Should I simply publish >> V3 >> and wait for feedback from the different vendors? (I would hate to >> cause a >> simular situation again) >> >> 3. Should I post V3 on top of drm-next or drm-misc-next? > > The normal rule is: always work on drm-tip. That is what gets > tested by our CI as well. Yes, it does mean a bit of extra hurdles > during development since drm-tip is a rebasing tree, but there are > tools like dim retip to help out here. > > As for where to merge them. I would generally recommed against merging > i915 patches through drm-misc unless there is a very compelling reason > to do so. i915 is a fast moving target and if there are significant > changes coming in via drm-misc they usually will cause conflicts for > people during drm-tip rebuild. Also I would expect to see an ack > requested from i915 maintainers for merging anything significant via > drm-misc, which I don't think happened in this case. Indeed. All other things aside, it looks like it has enough conflict potential to warrant merging via drm-intel anyway. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [Intel-gfx] [PATCH v2 00/13] drm/hdcp: Pull HDCP auth/exchange/check into helpers
On Wed, 15 Sep 2021, Sean Paul wrote: > From: Sean Paul > > Hello again, > This is the second version of the HDCP helper patchset. See version 1 > here: https://patchwork.freedesktop.org/series/94623/ > > In this second version, I've fixed up the oopsies exposed by 0-day and > yamllint and incorporated early review feedback from the dt/dts reviews. > > Please take a look, I'll try to ping folks to get someone to review the i915 parts, but the general idea of moving common HDCP code from i915 to drm is, I hope obviously, Acked-by: Jani Nikula > > Sean > > Sean Paul (13): > drm/hdcp: Add drm_hdcp_atomic_check() > drm/hdcp: Avoid changing crtc state in hdcp atomic check > drm/hdcp: Update property value on content type and user changes > drm/hdcp: Expand HDCP helper library for enable/disable/check > drm/i915/hdcp: Consolidate HDCP setup/state cache > drm/i915/hdcp: Retain hdcp_capable return codes > drm/i915/hdcp: Use HDCP helpers for i915 > drm/msm/dpu_kms: Re-order dpu includes > drm/msm/dpu: Remove useless checks in dpu_encoder > drm/msm/dpu: Remove encoder->enable() hack > drm/msm/dp: Re-order dp_audio_put in deinit_sub_modules > dt-bindings: msm/dp: Add bindings for HDCP registers > drm/msm: Implement HDCP 1.x using the new drm HDCP helpers > > .../bindings/display/msm/dp-controller.yaml |7 +- > arch/arm64/boot/dts/qcom/sc7180.dtsi |4 +- > drivers/gpu/drm/drm_hdcp.c| 1197 - > drivers/gpu/drm/i915/display/intel_atomic.c |7 +- > drivers/gpu/drm/i915/display/intel_ddi.c | 29 +- > .../drm/i915/display/intel_display_debugfs.c | 11 +- > .../drm/i915/display/intel_display_types.h| 58 +- > drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 345 ++--- > drivers/gpu/drm/i915/display/intel_dp_mst.c | 17 +- > drivers/gpu/drm/i915/display/intel_hdcp.c | 1011 +++--- > drivers/gpu/drm/i915/display/intel_hdcp.h | 35 +- > drivers/gpu/drm/i915/display/intel_hdmi.c | 256 ++-- > drivers/gpu/drm/msm/Makefile |1 + > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 30 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |2 - > drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h |4 - > drivers/gpu/drm/msm/dp/dp_debug.c | 49 +- > drivers/gpu/drm/msm/dp/dp_debug.h |6 +- > drivers/gpu/drm/msm/dp/dp_display.c | 47 +- > drivers/gpu/drm/msm/dp/dp_display.h |5 + > drivers/gpu/drm/msm/dp/dp_drm.c | 68 +- > drivers/gpu/drm/msm/dp/dp_drm.h |5 + > drivers/gpu/drm/msm/dp/dp_hdcp.c | 433 ++ > drivers/gpu/drm/msm/dp/dp_hdcp.h | 27 + > drivers/gpu/drm/msm/dp/dp_parser.c| 22 +- > drivers/gpu/drm/msm/dp/dp_parser.h|4 + > drivers/gpu/drm/msm/dp/dp_reg.h | 44 +- > drivers/gpu/drm/msm/msm_atomic.c | 15 + > include/drm/drm_hdcp.h| 194 +++ > 30 files changed, 2561 insertions(+), 1389 deletions(-) > create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.c > create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.h -- Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH v3 0/6] add fixes to pass DP Link Layer compliance test cases
On Thu, 05 Aug 2021, Kuogee Hsieh wrote: > add fixes to pass DP Link Layer compliance test cases Nitpick, please also include the approriate subject prefix to the cover letter, e.g. "drm/msm" or "drm/msm/dp". Helps with mail filtering. :) BR, Jani. > > Kuogee Hsieh (6): > drm/msm/dp: use dp_ctrl_off_link_stream during PHY compliance test run > drm/msm/dp: reduce link rate if failed at link training 1 > drm/msm/dp: reset aux controller after dp_aux_cmd_fifo_tx() failed. > drm/msm/dp: replug event is converted into an unplug followed by an > plug events > drm/msm/dp: return correct edid checksum after corrupted edid checksum > read > drm/msm/dp: do not end dp link training until video is ready > > drivers/gpu/drm/msm/dp/dp_aux.c | 3 + > drivers/gpu/drm/msm/dp/dp_ctrl.c| 137 > +++- > drivers/gpu/drm/msm/dp/dp_display.c | 14 ++-- > drivers/gpu/drm/msm/dp/dp_panel.c | 9 ++- > 4 files changed, 102 insertions(+), 61 deletions(-) -- Jani Nikula, Intel Open Source Graphics Center
Re: [Freedreno] [PATCH v3] drm/dp_mst: Fix return code on sideband message failure
On Tue, 06 Jul 2021, Kuogee Hsieh wrote:
> From: Rajkumar Subbiah
>
> Commit 2f015ec6eab6 ("drm/dp_mst: Add sideband down request tracing +
> selftests") added some debug code for sideband message tracing. But
> it seems to have unintentionally changed the behavior on sideband message
> failure. It catches and returns failure only if DRM_UT_DP is enabled.
> Otherwise it ignores the error code and returns success. So on an MST
> unplug, the caller is unaware that the clear payload message failed and
> ends up waiting for 4 seconds for the response. Fixes the issue by
> returning the proper error code.
>
> Changes in V2:
> -- Revise commit text as review comment
> -- add Fixes text
>
> Changes in V3:
> -- remove "unlikely" optimization
>
> Fixes: 2f015ec6eab6 ("drm/dp_mst: Add sideband down request tracing +
> selftests")
>
> Signed-off-by: Rajkumar Subbiah
> Signed-off-by: Kuogee Hsieh
>
> Reviewed-by: Stephen Boyd
Reviewed-by: Jani Nikula
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 1590144..df91110 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2887,11 +2887,13 @@ static int process_single_tx_qlock(struct
> drm_dp_mst_topology_mgr *mgr,
> idx += tosend + 1;
>
> ret = drm_dp_send_sideband_msg(mgr, up, chunk, idx);
> - if (unlikely(ret) && drm_debug_enabled(DRM_UT_DP)) {
> - struct drm_printer p = drm_debug_printer(DBG_PREFIX);
> + if (ret) {
> + if (drm_debug_enabled(DRM_UT_DP)) {
> + struct drm_printer p = drm_debug_printer(DBG_PREFIX);
>
> - drm_printf(, "sideband msg failed to send\n");
> - drm_dp_mst_dump_sideband_msg_tx(, txmsg);
> + drm_printf(, "sideband msg failed to send\n");
> + drm_dp_mst_dump_sideband_msg_tx(, txmsg);
> + }
> return ret;
> }
--
Jani Nikula, Intel Open Source Graphics Center
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2] drm/dp_mst: Fix return code on sideband message failure
On Tue, 29 Jun 2021, Kuogee Hsieh wrote:
> From: Rajkumar Subbiah
>
> Commit 2f015ec6eab6 ("drm/dp_mst: Add sideband down request tracing +
> selftests") added some debug code for sideband message tracing. But
> it seems to have unintentionally changed the behavior on sideband message
> failure. It catches and returns failure only if DRM_UT_DP is enabled.
> Otherwise it ignores the error code and returns success. So on an MST
> unplug, the caller is unaware that the clear payload message failed and
> ends up waiting for 4 seconds for the response. Fixes the issue by
> returning the proper error code.
>
> Changes in V2:
> -- Revise commit text as review comment
> -- add Fixes text
>
> Fixes: 2f015ec6eab6 ("drm/dp_mst: Add sideband down request tracing +
> selftests")
>
> Signed-off-by: Rajkumar Subbiah
> Signed-off-by: Kuogee Hsieh
>
> Reviewed-by: Stephen Boyd
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 1590144..8d97430 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2887,11 +2887,13 @@ static int process_single_tx_qlock(struct
> drm_dp_mst_topology_mgr *mgr,
> idx += tosend + 1;
>
> ret = drm_dp_send_sideband_msg(mgr, up, chunk, idx);
> - if (unlikely(ret) && drm_debug_enabled(DRM_UT_DP)) {
> - struct drm_printer p = drm_debug_printer(DBG_PREFIX);
> + if (unlikely(ret)) {
> + if (drm_debug_enabled(DRM_UT_DP)) {
> + struct drm_printer p = drm_debug_printer(DBG_PREFIX);
>
> - drm_printf(, "sideband msg failed to send\n");
> - drm_dp_mst_dump_sideband_msg_tx(, txmsg);
> + drm_printf(, "sideband msg failed to send\n");
> + drm_dp_mst_dump_sideband_msg_tx(, txmsg);
> + }
> return ret;
> }
Seems like a sensible thing to do.
(I'd probably rip out the "unlikely" while at it, as it feels like
unnecessary optimization, but *shrug*.)
Reviewed-by: Jani Nikula
--
Jani Nikula, Intel Open Source Graphics Center
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [v3 2/2] backlight: Add DisplayPort aux backlight driver
gt; + "failed to register backlight (%d)\n", ret);
> + goto free_ddc;
> + }
> +
> + platform_set_drvdata(pdev, bd);
> +
> + return 0;
> +
> +free_ddc:
> + if (ddc)
> + put_device(>dev);
> +
> + return ret;
> +}
> +
> +static int dp_aux_backlight_remove(struct platform_device *pdev)
> +{
> + struct backlight_device *bd = platform_get_drvdata(pdev);
> + struct dp_aux_backlight *aux_bl = bl_get_data(bd);
> + struct i2c_adapter *ddc = _bl->aux->ddc;
> +
> + if (ddc)
> + put_device(>dev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id dp_aux_bl_of_match_table[] = {
> + { .compatible = "dp-aux-backlight"},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, dp_aux_bl_of_match_table);
> +
> +static struct platform_driver dp_aux_backlight_driver = {
> + .driver = {
> + .name = "dp-aux-backlight",
> + .of_match_table = dp_aux_bl_of_match_table,
> + },
> + .probe = dp_aux_backlight_probe,
> + .remove = dp_aux_backlight_remove,
> +
> +};
> +module_platform_driver(dp_aux_backlight_driver);
> +
> +MODULE_DESCRIPTION("DisplayPort aux backlight driver");
> +MODULE_LICENSE("GPL v2");
--
Jani Nikula, Intel Open Source Graphics Center
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [v1 0/3] drm: Add support for backlight control of eDP panel on ti-sn65dsi86 bridge
Cc: Lyude and drm-misc maintainers On Wed, 14 Apr 2021, Rajeev Nandan wrote: > The backlight level of an eDP panel can be controlled through the AUX > channel using DPCD registers of the panel. > > The capability for the Source device to adjust backlight characteristics > within the panel, using the Sink device DPCD registers is indicated by > the TCON_BACKLIGHT_ADJUSTMENT_CAPABLE bit in the EDP_GENERAL_CAPABILITY_1 > register (DPCD Address 701h, bit0). In this configuration, the eDP TCON > receives the backlight level information from the host, through the AUX > channel. i915 has had this capability for some years now, and work is in progress to extract the DP AUX backlight code to drm core as helpers [1]. There's much more to it than what's proposed here. Adding incompatible DP AUX code at this point would be a pretty bad outcome. For example, we can't tie backlight device register to DP AUX backlight, because there are modes where *both* the eDP PWM pin based backlight control and DP AUX backlight control are used *simultaneously*. The backlight device register needs to be in code that is aware of both. Granted, it was a mistake way back when to add this in i915 only, and it should've been lifted to drm much earlier. It would've been done by Lyude by now, but people were not happy about not using drm device based logging. And that has unfortunately lead to a pretty massive prep series [2]. Please look into the code added to drm helpers in [1], and see how that would work for you. BR, Jani. [1] http://lore.kernel.org/r/20210205234515.1216538-1-ly...@redhat.com [2] http://lore.kernel.org/r/20210419225523.184856-1-ly...@redhat.com > > The changes in this patch series do the following: > - Add drm_dp_aux_backlight_ APIs to support backlight control using DPCD > registers on the DisplayPort AUX channel. > The current version only supports backlight brightness control by the > EDP_BACKLIGHT_BRIGHTNESS_MSB/LSB registers (DPCD Addresses 722h-723h). > - Add support for backlight control of the eDP panel connected to the > ti-sn65dsi86 bridge. > > Rajeev Nandan (3): > drm/dp: Add DisplayPort aux backlight control support > dt-bindings: drm/bridge: ti-sn65dsi86: Document use-aux-backlight > drm/bridge: ti-sn65dsi86: Add DisplayPort aux backlight support > > .../bindings/display/bridge/ti,sn65dsi86.yaml | 8 + > drivers/gpu/drm/Kconfig| 8 + > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/bridge/Kconfig | 1 + > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 26 +++ > drivers/gpu/drm/drm_dp_aux_backlight.c | 191 > + > include/drm/drm_dp_aux_backlight.h | 29 > 7 files changed, 264 insertions(+) > create mode 100644 drivers/gpu/drm/drm_dp_aux_backlight.c > create mode 100644 include/drm/drm_dp_aux_backlight.h -- Jani Nikula, Intel Open Source Graphics Center ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 05/20] drm/dp: Add backpointer to drm_device in drm_dp_aux
On Fri, 26 Mar 2021, Lyude Paul wrote:
> * The @dev field should be set to a pointer to the device that implements
> the
> - * AUX channel.
> + * AUX channel. As well, the @drm_dev field should be set to the _device
> + * that will be using this AUX channel as early as possible. For many
> graphics
> + * drivers this should happen before drm_dp_aux_init(), however it's
> perfectly
> + * fine to set this field later so long as it's assigned before calling
> + * drm_dp_aux_register().
Perhaps add a follow-up patch to actually ensure this is the case in
drm_dp_aux_register()?
> *
> * The @name field may be used to specify the name of the I2C adapter. If
> set to
> * %NULL, dev_name() of @dev will be used.
> @@ -1877,6 +1883,7 @@ struct drm_dp_aux {
> const char *name;
> struct i2c_adapter ddc;
> struct device *dev;
> + struct drm_device *drm_dev;
Bikeshed, I would probably have called it just drm for brevity, but no
strong feelings.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 06/20] drm/i915: Introduce GEM object functions
On Thu, 13 Aug 2020, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in i915.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 9 -
> drivers/gpu/drm/i915/i915_drv.c | 10 ++
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/selftests/mock_gem_device.c | 3 ---
> 4 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index c8421fd9d2dc..bc15ee4f2bd5 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -41,7 +41,14 @@ static struct i915_global_object {
>
> struct drm_i915_gem_object *i915_gem_object_alloc(void)
> {
> - return kmem_cache_zalloc(global.slab_objects, GFP_KERNEL);
> + struct drm_i915_gem_object *obj;
> +
> + obj = kmem_cache_zalloc(global.slab_objects, GFP_KERNEL);
> + if (!obj)
> + return NULL;
> + obj->base.funcs = _gem_object_funcs;
> +
> + return obj;
> }
>
> void i915_gem_object_free(struct drm_i915_gem_object *obj)
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 068447f565a9..b09eee11c540 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1840,6 +1840,12 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
> DRM_IOCTL_DEF_DRV(I915_GEM_VM_DESTROY, i915_gem_vm_destroy_ioctl,
> DRM_RENDER_ALLOW),
> };
>
> +const struct drm_gem_object_funcs i915_gem_object_funcs = {
> + .free = i915_gem_free_object,
> + .close = i915_gem_close_object,
> + .export = i915_gem_prime_export,
> +};
> +
Any reason not to make this static in i915_gem_object.c next to its only
user?
BR,
Jani.
> static struct drm_driver driver = {
> /* Don't use MTRRs here; the Xserver or userspace app should
>* deal with them for Intel hardware.
> @@ -1853,12 +1859,8 @@ static struct drm_driver driver = {
> .lastclose = i915_driver_lastclose,
> .postclose = i915_driver_postclose,
>
> - .gem_close_object = i915_gem_close_object,
> - .gem_free_object_unlocked = i915_gem_free_object,
> -
> .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> - .gem_prime_export = i915_gem_prime_export,
> .gem_prime_import = i915_gem_prime_import,
>
> .dumb_create = i915_gem_dumb_create,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bacb4c762f5b..666db65fe69e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1736,6 +1736,7 @@ intel_ggtt_update_needs_vtd_wa(struct drm_i915_private
> *dev_priv)
>
> /* i915_drv.c */
> extern const struct dev_pm_ops i915_pm_ops;
> +extern const struct drm_gem_object_funcs i915_gem_object_funcs;
>
> int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
> void i915_driver_remove(struct drm_i915_private *i915);
> diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> index ce4d4303229c..4725dad63e0a 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> @@ -86,9 +86,6 @@ static struct drm_driver mock_driver = {
> .name = "mock",
> .driver_features = DRIVER_GEM,
> .release = mock_device_release,
> -
> - .gem_close_object = i915_gem_close_object,
> - .gem_free_object_unlocked = i915_gem_free_object,
> };
>
> static void release_dev(struct device *dev)
--
Jani Nikula, Intel Open Source Graphics Center
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v6 2/5] drm: add constant N value in helper file
On Mon, 08 Jun 2020, Tanmay Shah wrote: > From: Chandan Uddaraju > > The constant N value (0x8000) is used by i915 DP > driver. Define this value in dp helper header file > to use in multiple Display Port drivers. Change > i915 driver accordingly. > > Change in v6: Change commit message > > Signed-off-by: Chandan Uddaraju > Signed-off-by: Vara Reddy > Signed-off-by: Tanmay Shah Acked-by: Jani Nikula for merging via drm-misc if that helps you. > --- > drivers/gpu/drm/i915/display/intel_display.c | 2 +- > include/drm/drm_dp_helper.h | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 9ea1a39..4b2cfff 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -8085,7 +8085,7 @@ static void compute_m_n(unsigned int m, unsigned int n, >* which the devices expect also in synchronous clock mode. >*/ > if (constant_n) > - *ret_n = 0x8000; > + *ret_n = DP_LINK_CONSTANT_N_VALUE; > else > *ret_n = min_t(unsigned int, roundup_pow_of_two(n), > DATA_LINK_N_MAX); > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index 2035ac4..589132a 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -1134,6 +1134,7 @@ > #define DP_MST_PHYSICAL_PORT_0 0 > #define DP_MST_LOGICAL_PORT_0 8 > > +#define DP_LINK_CONSTANT_N_VALUE 0x8000 > #define DP_LINK_STATUS_SIZE 6 > bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE], > int lane_count); -- Jani Nikula, Intel Open Source Graphics Center ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH][next] drm: Replace zero-length array with flexible-array member
On Tue, 25 Feb 2020, "Gustavo A. R. Silva" wrote:
> The current codebase makes use of the zero-length array language
> extension to the C90 standard, but the preferred mechanism to declare
> variable-length types such as these ones is a flexible array member[1][2],
> introduced in C99:
>
> struct foo {
> int stuff;
> struct boo array[];
> };
>
> By making use of the mechanism above, we will get a compiler warning
> in case the flexible array does not occur last in the structure, which
> will help us prevent some kind of undefined behavior bugs from being
> inadvertently introduced[3] to the codebase from now on.
>
> Also, notice that, dynamic memory allocations won't be affected by
> this change:
>
> "Flexible array members have incomplete type, and so the sizeof operator
> may not be applied. As a quirk of the original implementation of
> zero-length arrays, sizeof evaluates to zero."[1]
>
> This issue was found with the help of Coccinelle.
>
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://github.com/KSPP/linux/issues/21
> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
>
> Signed-off-by: Gustavo A. R. Silva
> ---
> drivers/gpu/drm/etnaviv/etnaviv_gem.h | 2 +-
> drivers/gpu/drm/gma500/intel_bios.h | 2 +-
> drivers/gpu/drm/i915/display/intel_vbt_defs.h | 4 ++--
> drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +-
> drivers/gpu/drm/i915/i915_gpu_error.h | 2 +-
Please split out the i915 changes to a separate patch.
> drivers/gpu/drm/msm/msm_gem.h | 2 +-
> drivers/gpu/drm/qxl/qxl_cmd.c | 2 +-
> drivers/gpu/drm/vboxvideo/vboxvideo.h | 2 +-
> drivers/gpu/drm/vc4/vc4_drv.h | 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c| 2 +-
> drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 2 +-
> include/drm/bridge/mhl.h | 4 ++--
> include/drm/drm_displayid.h | 2 +-
> include/uapi/drm/i915_drm.h | 4 ++--
Not sure it's worth touching uapi headers. They're full of both [0] and
[]. Again, please at least split it to a separate patch to be decided
separately.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 07/21] drm/i915: Convert to CRTC VBLANK callbacks
On Wed, 15 Jan 2020, Thomas Zimmermann wrote:
> VBLANK callbacks in struct drm_driver are deprecated in favor of their
> equivalents in struct drm_crtc_funcs. Convert i915 over.
>
> The callback struct drm_driver.get_scanout_position() is deprecated
> in favor of struct drm_crtc_helper_funcs.get_scanout_position().
> i915 doesn't use CRTC helpers. Instead pass i915's implementation of
> get_scanout_position() to DRM core's
> drm_crtc_vblank_helper_get_vblank_timestamp_internal().
>
> v2:
> * use DRM's implementation of get_vblank_timestamp()
> * simplify function names
>
> Signed-off-by: Thomas Zimmermann
Acked-by: Jani Nikula
for the approach and for merging through whichever tree makes most
sense, *however* needs detailed review on the whole.
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 7 +++
> drivers/gpu/drm/i915/i915_drv.c | 3 ---
> drivers/gpu/drm/i915/i915_irq.c | 20 +++-
> drivers/gpu/drm/i915/i915_irq.h | 6 ++
> 4 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 59c375879186..c8f1da845e7d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -16336,6 +16336,7 @@ static const struct drm_crtc_funcs bdw_crtc_funcs = {
> .get_vblank_counter = g4x_get_vblank_counter,
> .enable_vblank = bdw_enable_vblank,
> .disable_vblank = bdw_disable_vblank,
> + .get_vblank_timestamp = i915_crtc_get_vblank_timestamp,
> };
>
> static const struct drm_crtc_funcs ilk_crtc_funcs = {
> @@ -16344,6 +16345,7 @@ static const struct drm_crtc_funcs ilk_crtc_funcs = {
> .get_vblank_counter = g4x_get_vblank_counter,
> .enable_vblank = ilk_enable_vblank,
> .disable_vblank = ilk_disable_vblank,
> + .get_vblank_timestamp = i915_crtc_get_vblank_timestamp,
> };
>
> static const struct drm_crtc_funcs g4x_crtc_funcs = {
> @@ -16352,6 +16354,7 @@ static const struct drm_crtc_funcs g4x_crtc_funcs = {
> .get_vblank_counter = g4x_get_vblank_counter,
> .enable_vblank = i965_enable_vblank,
> .disable_vblank = i965_disable_vblank,
> + .get_vblank_timestamp = i915_crtc_get_vblank_timestamp,
> };
>
> static const struct drm_crtc_funcs i965_crtc_funcs = {
> @@ -16360,6 +16363,7 @@ static const struct drm_crtc_funcs i965_crtc_funcs = {
> .get_vblank_counter = i915_get_vblank_counter,
> .enable_vblank = i965_enable_vblank,
> .disable_vblank = i965_disable_vblank,
> + .get_vblank_timestamp = i915_crtc_get_vblank_timestamp,
> };
>
> static const struct drm_crtc_funcs i915gm_crtc_funcs = {
> @@ -16368,6 +16372,7 @@ static const struct drm_crtc_funcs i915gm_crtc_funcs
> = {
> .get_vblank_counter = i915_get_vblank_counter,
> .enable_vblank = i915gm_enable_vblank,
> .disable_vblank = i915gm_disable_vblank,
> + .get_vblank_timestamp = i915_crtc_get_vblank_timestamp,
> };
>
> static const struct drm_crtc_funcs i915_crtc_funcs = {
> @@ -16376,6 +16381,7 @@ static const struct drm_crtc_funcs i915_crtc_funcs = {
> .get_vblank_counter = i915_get_vblank_counter,
> .enable_vblank = i8xx_enable_vblank,
> .disable_vblank = i8xx_disable_vblank,
> + .get_vblank_timestamp = i915_crtc_get_vblank_timestamp,
> };
>
> static const struct drm_crtc_funcs i8xx_crtc_funcs = {
> @@ -16384,6 +16390,7 @@ static const struct drm_crtc_funcs i8xx_crtc_funcs = {
> /* no hw vblank counter */
> .enable_vblank = i8xx_enable_vblank,
> .disable_vblank = i8xx_disable_vblank,
> + .get_vblank_timestamp = i915_crtc_get_vblank_timestamp,
> };
>
> static struct intel_crtc *intel_crtc_alloc(void)
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f7385abdd74b..30b9ba136a81 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -2769,9 +2769,6 @@ static struct drm_driver driver = {
> .gem_prime_export = i915_gem_prime_export,
> .gem_prime_import = i915_gem_prime_import,
>
> - .get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos,
> - .get_scanout_position = i915_get_crtc_scanoutpos,
> -
> .dumb_create = i915_gem_dumb_create,
> .dumb_map_offset = i915_gem_dumb_mmap_offset,
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index afc6aad9bf8c..c39e3ef6e4a2 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -762,13 +762,15 @@ static int __intel_ge
Re: [Freedreno] [PATCH 03/23] drm/i915: Don't use struct drm_driver.get_scanout_position()
On Fri, 10 Jan 2020, Thomas Zimmermann wrote:
> Hi
>
> Am 10.01.20 um 12:59 schrieb Jani Nikula:
>> On Fri, 10 Jan 2020, Thomas Zimmermann wrote:
>>> The callback struct drm_driver.get_scanout_position() is deprecated in
>>> favor of struct drm_crtc_helper_funcs.get_scanout_position().
>>>
>>> i915 doesn't use CRTC helpers. The patch duplicates the caller
>>> drm_calc_vbltimestamp_from_scanoutpos() for i915, such that the callback
>>> function is not needed.
>>>
>>> Signed-off-by: Thomas Zimmermann
>>> ---
>>> drivers/gpu/drm/i915/i915_drv.c | 3 +-
>>> drivers/gpu/drm/i915/i915_irq.c | 117 ++--
>>> drivers/gpu/drm/i915/i915_irq.h | 9 +--
>>> 3 files changed, 119 insertions(+), 10 deletions(-)
>>
>> Not really enthusiastic about the diffstat in a "cleanup" series.
>
> Well, the cleanup is about the content of drm_driver :)
>
>>
>> I wonder if you could add a generic helper version of
>> drm_calc_vbltimestamp_from_scanoutpos where you pass the
>> get_scanout_position function as a parameter. Both
>> drm_calc_vbltimestamp_from_scanoutpos and the new
>> i915_calc_vbltimestamp_from_scanoutpos would then be fairly thin
>> wrappers passing in the relevant get_scanout_position function.
>
> Of course. Will be in v2 of the series.
Please give Ville (Cc'd) a moment before sending v2 in case he wants to
chime in on this.
Thanks,
Jani.
>
> Best regards
> Thomas
>
>>
>> This would reduce the almost identical duplication of the function in
>> i915.
>>
>> BR,
>> Jani.
>>
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>>> b/drivers/gpu/drm/i915/i915_drv.c
>>> index f7385abdd74b..4a0a7fb85c53 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -2769,8 +2769,7 @@ static struct drm_driver driver = {
>>> .gem_prime_export = i915_gem_prime_export,
>>> .gem_prime_import = i915_gem_prime_import,
>>>
>>> - .get_vblank_timestamp = drm_calc_vbltimestamp_from_scanoutpos,
>>> - .get_scanout_position = i915_get_crtc_scanoutpos,
>>> + .get_vblank_timestamp = i915_calc_vbltimestamp_from_scanoutpos,
>>>
>>> .dumb_create = i915_gem_dumb_create,
>>> .dumb_map_offset = i915_gem_dumb_mmap_offset,
>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>>> b/drivers/gpu/drm/i915/i915_irq.c
>>> index afc6aad9bf8c..99d0c3b0feae 100644
>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>> @@ -52,6 +52,11 @@
>>> #include "i915_trace.h"
>>> #include "intel_pm.h"
>>>
>>> +/* Retry timestamp calculation up to 3 times to satisfy
>>> + * drm_timestamp_precision before giving up.
>>> + */
>>> +#define I915_TIMESTAMP_MAXRETRIES 3
>>> +
>>> /**
>>> * DOC: interrupt handling
>>> *
>>> @@ -762,10 +767,11 @@ static int __intel_get_crtc_scanline(struct
>>> intel_crtc *crtc)
>>> return (position + crtc->scanline_offset) % vtotal;
>>> }
>>>
>>> -bool i915_get_crtc_scanoutpos(struct drm_device *dev, unsigned int index,
>>> - bool in_vblank_irq, int *vpos, int *hpos,
>>> - ktime_t *stime, ktime_t *etime,
>>> - const struct drm_display_mode *mode)
>>> +static bool i915_get_crtc_scanoutpos(struct drm_device *dev,
>>> +unsigned int index, bool in_vblank_irq,
>>> +int *vpos, int *hpos,
>>> +ktime_t *stime, ktime_t *etime,
>>> +const struct drm_display_mode *mode)
>>> {
>>> struct drm_i915_private *dev_priv = to_i915(dev);
>>> struct intel_crtc *crtc = to_intel_crtc(drm_crtc_from_index(dev,
>>> index));
>>> @@ -879,6 +885,109 @@ bool i915_get_crtc_scanoutpos(struct drm_device *dev,
>>> unsigned int index,
>>> return true;
>>> }
>>>
>>> +bool i915_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
>>> + unsigned int pipe,
>>> + int *max_error,
>>> + ktime_t *vblank_time,
>>> + bool in_vblank_irq)
>>>
Re: [Freedreno] [PATCH 01/23] drm: Add get_scanout_position() to struct drm_crtc_helper_funcs
; + > + /** > + * @get_scanout_position: > + * > + * Called by vblank timestamping code. > + * > + * Returns the current display scanout position from a CRTC and an > + * optional accurate ktime_get() timestamp of when the position was > + * measured. Note that this is a helper callback which is only used > + * if a driver uses drm_calc_vbltimestamp_from_scanoutpos() for the > + * @drm_driver.get_vblank_timestamp callback. > + * > + * Parameters: > + * > + * crtc: > + * The CRTC. > + * in_vblank_irq: > + * True when called from drm_crtc_handle_vblank(). Some drivers > + * need to apply some workarounds for gpu-specific vblank irq > + * quirks if the flag is set. > + * vpos: > + * Target location for current vertical scanout position. > + * hpos: > + * Target location for current horizontal scanout position. > + * stime: > + * Target location for timestamp taken immediately before > + * scanout position query. Can be NULL to skip timestamp. > + * etime: > + * Target location for timestamp taken immediately after > + * scanout position query. Can be NULL to skip timestamp. > + * mode: > + * Current display timings. > + * > + * Returns vpos as a positive number while in active scanout area. > + * Returns vpos as a negative number inside vblank, counting the number > + * of scanlines to go until end of vblank, e.g., -1 means "one scanline > + * until start of active scanout / end of vblank." > + * > + * Returns: > + * > + * True on success, false if a reliable scanout position counter could > + * not be read out. > + */ > + bool (*get_scanout_position)(struct drm_crtc *crtc, > + bool in_vblank_irq, int *vpos, int *hpos, > + ktime_t *stime, ktime_t *etime, > + const struct drm_display_mode *mode); > }; > > /** -- Jani Nikula, Intel Open Source Graphics Center ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [DPU PATCH v3 2/5] drm: add constant N value in helper file
On Mon, 02 Dec 2019, Chandan Uddaraju wrote:
> The constant N value (0x8000) is used by multiple DP
> drivers. Define this value in header file and use this
> in the existing i915 display driver.
>
> Signed-off-by: Chandan Uddaraju
Reviewed-by: Jani Nikula
and ack for merging via drm-misc or whichever tree you find suitable.
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 2 +-
> include/drm/drm_dp_helper.h | 1 +
> 2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index ce05e80..1a4ccfd 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7496,7 +7496,7 @@ static void compute_m_n(unsigned int m, unsigned int n,
>* which the devices expect also in synchronous clock mode.
>*/
> if (constant_n)
> - *ret_n = 0x8000;
> + *ret_n = DP_LINK_CONSTANT_N_VALUE;
> else
> *ret_n = min_t(unsigned int, roundup_pow_of_two(n),
> DATA_LINK_N_MAX);
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 8364502..69b8251 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -1357,6 +1357,7 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
> * DisplayPort link
> */
> #define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
> +#define DP_LINK_CONSTANT_N_VALUE 0x8000
>
> struct drm_dp_link {
> unsigned char revision;
--
Jani Nikula, Intel Open Source Graphics Center
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 0/9] drm/print: add and use drm_debug_enabled()
On Tue, 24 Sep 2019, Jani Nikula wrote: > drm/print: move drm_debug variable to drm_print.[ch] > drm/print: add drm_debug_enabled() > drm/etnaviv: use drm_debug_enabled() to check for debug categories > drm/i2c/sil164: use drm_debug_enabled() to check for debug categories > drm/msm: use drm_debug_enabled() to check for debug categories Pushed the above patches to drm-misc-next, thanks for the reviews and acks! The below i915 and amdgpu patches conflict on drm-misc-next, and I haven't received any feedback on nouveau. > drm/i915: use drm_debug_enabled() to check for debug categories > drm/nouveau: use drm_debug_enabled() to check for debug categories > drm/amdgpu: use drm_debug_enabled() to check for debug categories > drm/print: rename drm_debug to __drm_debug to discourage use BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 0/9] drm/print: add and use drm_debug_enabled()
On Tue, 01 Oct 2019, Eric Engestrom wrote:
> On Tuesday, 2019-10-01 14:03:55 +0300, Jani Nikula wrote:
>> On Thu, 26 Sep 2019, Eric Engestrom wrote:
>> > On Tuesday, 2019-09-24 15:58:56 +0300, Jani Nikula wrote:
>> >> Hi all, v2 of [1], a little refactoring around drm_debug access to
>> >> abstract it better. There shouldn't be any functional changes.
>> >>
>> >> I'd appreciate acks for merging the lot via drm-misc. If there are any
>> >> objections to that, we'll need to postpone the last patch until
>> >> everything has been merged and converted in drm-next.
>> >>
>> >> BR,
>> >> Jani.
>> >>
>> >> Cc: Eric Engestrom
>> >> Cc: Alex Deucher
>> >> Cc: Christian König
>> >> Cc: David (ChunMing) Zhou
>> >> Cc: amd-...@lists.freedesktop.org
>> >> Cc: Ben Skeggs
>> >> Cc: nouv...@lists.freedesktop.org
>> >> Cc: Rob Clark
>> >> Cc: Sean Paul
>> >> Cc: linux-arm-...@vger.kernel.org
>> >> Cc: freedreno@lists.freedesktop.org
>> >> Cc: Francisco Jerez
>> >> Cc: Lucas Stach
>> >> Cc: Russell King
>> >> Cc: Christian Gmeiner
>> >> Cc: etna...@lists.freedesktop.org
>> >>
>> >>
>> >> [1] cover.1568375189.git.jani.nikula@intel.com">http://mid.mail-archive.com/cover.1568375189.git.jani.nikula@intel.com
>> >>
>> >> Jani Nikula (9):
>> >> drm/print: move drm_debug variable to drm_print.[ch]
>> >> drm/print: add drm_debug_enabled()
>> >> drm/i915: use drm_debug_enabled() to check for debug categories
>> >> drm/print: rename drm_debug to __drm_debug to discourage use
>> >
>> > The above four patches are:
>> > Reviewed-by: Eric Engestrom
>> >
>> > Did you check to make sure the `unlikely()` is propagated correctly
>> > outside the `drm_debug_enabled()` call?
>>
>> I did now.
>>
>> Having drm_debug_enabled() as a macro vs. as an inline function does not
>> seem to make a difference, so I think the inline is clearly preferrable.
>
> Agreed :)
>
>>
>> However, for example
>>
>> unlikely(foo && drm_debug & DRM_UT_DP)
>>
>> does produce code different from
>>
>> (foo && drm_debug_enabled(DRM_UT_DP))
>>
>> indicating that the unlikely() within drm_debug_enabled() does not
>> propagate to the whole condition. It's possible to retain the same
>> assembly output with
>>
>> (unlikely(foo) && drm_debug_enabled(DRM_UT_DP))
>>
>> but it's unclear to me whether this is really worth it, either
>> readability or performance wise.
>>
>> Thoughts?
>
> That kind of code only happens 2 times, both in
> drivers/gpu/drm/drm_dp_mst_topology.c (in patch 2/9), right?
>
> I think your suggestion is the right thing to do here:
>
> - if (unlikely(ret && drm_debug & DRM_UT_DP)) {
> + if (unlikely(ret) && drm_debug_enabled(DRM_UT_DP)) {
>
> It doesn't really cost much in readability (especially compared to what
> it was before), and whether it's important performance wise I couldn't
> tell, but I think it's best to keep the code optimised as it was before
> unless there's a reason to drop it.
>
> Lyude might know more since she wrote 2f015ec6eab69301fdcf5, if you want
> to ping her?
Just ended up sending the updated version with what you suggest and I
agree with; pedantically the change should be a separate patch anyway.
Thanks for your inputs.
BR,
Jani.
>
>>
>> BR,
>> Jani.
>>
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center
--
Jani Nikula, Intel Open Source Graphics Center
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 0/9] drm/print: add and use drm_debug_enabled()
On Thu, 26 Sep 2019, Eric Engestrom wrote: > On Tuesday, 2019-09-24 15:58:56 +0300, Jani Nikula wrote: >> Hi all, v2 of [1], a little refactoring around drm_debug access to >> abstract it better. There shouldn't be any functional changes. >> >> I'd appreciate acks for merging the lot via drm-misc. If there are any >> objections to that, we'll need to postpone the last patch until >> everything has been merged and converted in drm-next. >> >> BR, >> Jani. >> >> Cc: Eric Engestrom >> Cc: Alex Deucher >> Cc: Christian König >> Cc: David (ChunMing) Zhou >> Cc: amd-...@lists.freedesktop.org >> Cc: Ben Skeggs >> Cc: nouv...@lists.freedesktop.org >> Cc: Rob Clark >> Cc: Sean Paul >> Cc: linux-arm-...@vger.kernel.org >> Cc: freedreno@lists.freedesktop.org >> Cc: Francisco Jerez >> Cc: Lucas Stach >> Cc: Russell King >> Cc: Christian Gmeiner >> Cc: etna...@lists.freedesktop.org >> >> >> [1] cover.1568375189.git.jani.nikula@intel.com">http://mid.mail-archive.com/cover.1568375189.git.jani.nikula@intel.com >> >> Jani Nikula (9): >> drm/print: move drm_debug variable to drm_print.[ch] >> drm/print: add drm_debug_enabled() >> drm/i915: use drm_debug_enabled() to check for debug categories >> drm/print: rename drm_debug to __drm_debug to discourage use > > The above four patches are: > Reviewed-by: Eric Engestrom > > Did you check to make sure the `unlikely()` is propagated correctly > outside the `drm_debug_enabled()` call? I did now. Having drm_debug_enabled() as a macro vs. as an inline function does not seem to make a difference, so I think the inline is clearly preferrable. However, for example unlikely(foo && drm_debug & DRM_UT_DP) does produce code different from (foo && drm_debug_enabled(DRM_UT_DP)) indicating that the unlikely() within drm_debug_enabled() does not propagate to the whole condition. It's possible to retain the same assembly output with (unlikely(foo) && drm_debug_enabled(DRM_UT_DP)) but it's unclear to me whether this is really worth it, either readability or performance wise. Thoughts? BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v2 6/9] drm/msm: use drm_debug_enabled() to check for debug categories
Allow better abstraction of the drm_debug global variable in the
future. No functional changes.
v2: Move unlikely() to drm_debug_enabled()
Cc: Rob Clark
Cc: Sean Paul
Cc: linux-arm-...@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Reviewed-by: Rob Clark
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 4c889aabdaf9..959d03e007fa 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -31,7 +31,7 @@
*/
#define DPU_DEBUG(fmt, ...)\
do { \
- if (unlikely(drm_debug & DRM_UT_KMS)) \
+ if (drm_debug_enabled(DRM_UT_KMS)) \
DRM_DEBUG(fmt, ##__VA_ARGS__); \
else \
pr_debug(fmt, ##__VA_ARGS__); \
@@ -43,7 +43,7 @@
*/
#define DPU_DEBUG_DRIVER(fmt, ...) \
do { \
- if (unlikely(drm_debug & DRM_UT_DRIVER)) \
+ if (drm_debug_enabled(DRM_UT_DRIVER)) \
DRM_ERROR(fmt, ##__VA_ARGS__); \
else \
pr_debug(fmt, ##__VA_ARGS__); \
--
2.20.1
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH v2 0/9] drm/print: add and use drm_debug_enabled()
Hi all, v2 of [1], a little refactoring around drm_debug access to abstract it better. There shouldn't be any functional changes. I'd appreciate acks for merging the lot via drm-misc. If there are any objections to that, we'll need to postpone the last patch until everything has been merged and converted in drm-next. BR, Jani. Cc: Eric Engestrom Cc: Alex Deucher Cc: Christian König Cc: David (ChunMing) Zhou Cc: amd-...@lists.freedesktop.org Cc: Ben Skeggs Cc: nouv...@lists.freedesktop.org Cc: Rob Clark Cc: Sean Paul Cc: linux-arm-...@vger.kernel.org Cc: freedreno@lists.freedesktop.org Cc: Francisco Jerez Cc: Lucas Stach Cc: Russell King Cc: Christian Gmeiner Cc: etna...@lists.freedesktop.org [1] cover.1568375189.git.jani.nikula@intel.com">http://mid.mail-archive.com/cover.1568375189.git.jani.nikula@intel.com Jani Nikula (9): drm/print: move drm_debug variable to drm_print.[ch] drm/print: add drm_debug_enabled() drm/etnaviv: use drm_debug_enabled() to check for debug categories drm/i2c/sil164: use drm_debug_enabled() to check for debug categories drm/i915: use drm_debug_enabled() to check for debug categories drm/msm: use drm_debug_enabled() to check for debug categories drm/nouveau: use drm_debug_enabled() to check for debug categories drm/amdgpu: use drm_debug_enabled() to check for debug categories drm/print: rename drm_debug to __drm_debug to discourage use drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c | 4 ++-- drivers/gpu/drm/drm_atomic_uapi.c| 2 +- drivers/gpu/drm/drm_dp_mst_topology.c| 6 ++--- drivers/gpu/drm/drm_drv.c| 17 --- drivers/gpu/drm/drm_edid.c | 2 +- drivers/gpu/drm/drm_edid_load.c | 2 +- drivers/gpu/drm/drm_mipi_dbi.c | 4 ++-- drivers/gpu/drm/drm_print.c | 23 ++-- drivers/gpu/drm/drm_vblank.c | 6 ++--- drivers/gpu/drm/etnaviv/etnaviv_buffer.c | 8 +++ drivers/gpu/drm/i2c/sil164_drv.c | 2 +- drivers/gpu/drm/i915/display/intel_display.c | 4 ++-- drivers/gpu/drm/i915/display/intel_dp.c | 2 +- drivers/gpu/drm/i915/i915_drv.c | 2 +- drivers/gpu/drm/i915/i915_gem.h | 2 +- drivers/gpu/drm/i915/i915_utils.c| 2 +- drivers/gpu/drm/i915/intel_pm.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 ++-- drivers/gpu/drm/nouveau/dispnv50/disp.h | 4 ++-- drivers/gpu/drm/nouveau/nouveau_drv.h| 4 ++-- include/drm/drm_drv.h| 2 -- include/drm/drm_print.h | 8 +++ 22 files changed, 60 insertions(+), 52 deletions(-) -- 2.20.1 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH 6/9] drm/msm: use drm_debug_enabled() to check for debug categories
Allow better abstraction of the drm_debug global variable in the
future. No functional changes.
Cc: Rob Clark
Cc: Sean Paul
Cc: linux-arm-...@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 9e40f559c51f..00e3353f9aad 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -29,7 +29,7 @@
*/
#define DPU_DEBUG(fmt, ...)\
do { \
- if (unlikely(drm_debug & DRM_UT_KMS)) \
+ if (unlikely(drm_debug_enabled(DRM_UT_KMS))) \
DRM_DEBUG(fmt, ##__VA_ARGS__); \
else \
pr_debug(fmt, ##__VA_ARGS__); \
@@ -41,7 +41,7 @@
*/
#define DPU_DEBUG_DRIVER(fmt, ...) \
do { \
- if (unlikely(drm_debug & DRM_UT_DRIVER)) \
+ if (unlikely(drm_debug_enabled(DRM_UT_DRIVER)))\
DRM_ERROR(fmt, ##__VA_ARGS__); \
else \
pr_debug(fmt, ##__VA_ARGS__); \
--
2.20.1
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH 0/9] drm/print: add and use drm_debug_enabled()
Hi all, just a little refactoring around drm_debug access to abstract it better. There shouldn't be any functional changes. I'd appreciate acks for merging the lot via drm-misc. If there are any objections to that, we'll need to postpone the last patch until everything has been merged and converted in drm-next. BR, Jani. Cc: Alex Deucher Cc: Christian König Cc: David (ChunMing) Zhou Cc: amd-...@lists.freedesktop.org Cc: Ben Skeggs Cc: nouv...@lists.freedesktop.org Cc: Rob Clark Cc: Sean Paul Cc: linux-arm-...@vger.kernel.org Cc: freedreno@lists.freedesktop.org Cc: Francisco Jerez Cc: Lucas Stach Cc: Russell King Cc: Christian Gmeiner Cc: etna...@lists.freedesktop.org Jani Nikula (9): drm/print: move drm_debug variable to drm_print.[ch] drm/print: add drm_debug_enabled() drm/etnaviv: use drm_debug_enabled() to check for debug categories drm/i2c/sil164: use drm_debug_enabled() to check for debug categories drm/i915: use drm_debug_enabled() to check for debug categories drm/msm: use drm_debug_enabled() to check for debug categories drm/nouveau: use drm_debug_enabled() to check for debug categories drm/amdgpu: use drm_debug_enabled() to check for debug categories drm/print: rename drm_debug to __drm_debug to discourage use drivers/gpu/drm/amd/amdgpu/smu_v11_0_i2c.c | 4 ++-- drivers/gpu/drm/drm_atomic_uapi.c| 2 +- drivers/gpu/drm/drm_dp_mst_topology.c| 6 ++--- drivers/gpu/drm/drm_drv.c| 17 --- drivers/gpu/drm/drm_edid.c | 2 +- drivers/gpu/drm/drm_edid_load.c | 2 +- drivers/gpu/drm/drm_mipi_dbi.c | 4 ++-- drivers/gpu/drm/drm_print.c | 23 ++-- drivers/gpu/drm/drm_vblank.c | 6 ++--- drivers/gpu/drm/etnaviv/etnaviv_buffer.c | 8 +++ drivers/gpu/drm/i2c/sil164_drv.c | 2 +- drivers/gpu/drm/i915/display/intel_display.c | 4 ++-- drivers/gpu/drm/i915/display/intel_dp.c | 2 +- drivers/gpu/drm/i915/i915_drv.c | 2 +- drivers/gpu/drm/i915/i915_gem.h | 2 +- drivers/gpu/drm/i915/i915_utils.c| 2 +- drivers/gpu/drm/i915/intel_pm.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 4 ++-- drivers/gpu/drm/nouveau/dispnv50/disp.h | 4 ++-- drivers/gpu/drm/nouveau/nouveau_drv.h| 4 ++-- include/drm/drm_drv.h| 2 -- include/drm/drm_print.h | 8 +++ 22 files changed, 60 insertions(+), 52 deletions(-) -- 2.20.1 ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v5 02/24] drm: Add drm_connector_init() variant with ddc
On Wed, 24 Jul 2019, Andrzej Pietrasiewicz wrote:
> Allow passing ddc adapter pointer to the init function. Even if
> drm_connector_init() sometime in the future decides to e.g. memset() all
> connector fields to zeros, the newly added function ensures that at its
> completion the ddc member of connector is correctly set.
>
> Signed-off-by: Andrzej Pietrasiewicz
> ---
> drivers/gpu/drm/drm_connector.c | 19 +++
> include/drm/drm_connector.h | 5 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 068d4b05f1be..06fbfc44fb48 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -296,6 +296,25 @@ int drm_connector_init(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_connector_init);
>
> +int drm_connector_init_with_ddc(struct drm_device *dev,
> + struct drm_connector *connector,
> + const struct drm_connector_funcs *funcs,
> + int connector_type,
> + struct i2c_adapter *ddc)
Playing the devil's advocate here a bit. What if you end up adding
another thing you need to initialize like this? Are you going to need
three more functions to account for the combinations? Init with ddc,
with foo, with ddc and foo? So I generally frown upon interfaces like
this.
If everyone thinks this is the way to go, I'm not going to stand in the
way, but personally I'd rather switch over all of i915 to a new version
of drm_connector_init() that just takes another parameter.
BR,
Jani.
> +{
> + int ret;
> +
> + ret = drm_connector_init(dev, connector, funcs, connector_type);
> + if (ret)
> + return ret;
> +
> + /* provide ddc symlink in sysfs */
> + connector->ddc = ddc;
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_connector_init_with_ddc);
> +
> /**
> * drm_connector_attach_edid_property - attach edid property.
> * @connector: the connector
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 33a6fff85fdb..937fda9c1374 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1410,6 +1410,11 @@ int drm_connector_init(struct drm_device *dev,
> struct drm_connector *connector,
> const struct drm_connector_funcs *funcs,
> int connector_type);
> +int drm_connector_init_with_ddc(struct drm_device *dev,
> + struct drm_connector *connector,
> + const struct drm_connector_funcs *funcs,
> + int connector_type,
> + struct i2c_adapter *ddc);
> void drm_connector_attach_edid_property(struct drm_connector *connector);
> int drm_connector_register(struct drm_connector *connector);
> void drm_connector_unregister(struct drm_connector *connector);
--
Jani Nikula, Intel Open Source Graphics Center
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] Cleanup of -Wunused-const-variable in drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
On Thu, 13 Jun 2019, Nathan Huckleberry wrote:
> Hey all,
>
> I'm looking into cleaning up ignored warnings in the kernel so we can
> remove compiler flags to ignore warnings.
Wholeheartedly agreed on the goal.
> There are several unused variables in dpu_formats.c
> ('dpu_format_map_tile', 'dpu_format_map_p010',
> 'dpu_format_map_p010_ubwc', 'dpu_format_map_tp10_ubwc').
> They look like modifiers that were never implemented. I'd like to
> remove these variables if there are no plans moving forward to
> implement them. Otherwise I'll just leave them.
>
> https://github.com/ClangBuiltLinux/linux/issues/528
No opinion on the said variables above, but, FWIW, personally I think
it's fine to add the cflags to supress warnings locally where needed in
order to be able to achieve the greater goal of removing the cflags
globally.
In drivers/gpu/drm/i915/Makefile we actually go for much stricter
warnings than the kernel defaults, and disable a more limited and
tailored set of warnings.
You can do this both on a subdir and file level with subdir-ccflags-y
and CFLAGS_filename.o, respectively.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH] drm: remove redundant 'default n' from Kconfig
On Fri, 12 Apr 2019, Bartlomiej Zolnierkiewicz wrote:
> 'default n' is the default value for any bool or tristate Kconfig
> setting so there is no need to write it explicitly.
>
> Also since commit f467c5640c29 ("kconfig: only write '# CONFIG_FOO
> is not set' for visible symbols") the Kconfig behavior is the same
> regardless of 'default n' being present or not:
>
> ...
> One side effect of (and the main motivation for) this change is making
> the following two definitions behave exactly the same:
>
> config FOO
> bool
>
> config FOO
> bool
> default n
>
> With this change, neither of these will generate a
> '# CONFIG_FOO is not set' line (assuming FOO isn't selected/implied).
> That might make it clearer to people that a bare 'default n' is
> redundant.
> ...
>
> Signed-off-by: Bartlomiej Zolnierkiewicz
> ---
> drivers/gpu/drm/Kconfig |5 -
> drivers/gpu/drm/amd/amdgpu/Kconfig |1 -
> drivers/gpu/drm/arm/Kconfig |1 -
> drivers/gpu/drm/exynos/Kconfig |2 --
> drivers/gpu/drm/i915/Kconfig |3 ---
> drivers/gpu/drm/i915/Kconfig.debug | 13 -
For i915,
Acked-by: Jani Nikula
and can be merged through whichever tree you like.
> drivers/gpu/drm/msm/Kconfig |2 --
> drivers/gpu/drm/nouveau/Kconfig |2 --
> drivers/gpu/drm/omapdrm/Kconfig |1 -
> drivers/gpu/drm/omapdrm/dss/Kconfig |6 --
> 10 files changed, 36 deletions(-)
>
> Index: b/drivers/gpu/drm/Kconfig
> ===
> --- a/drivers/gpu/drm/Kconfig 2019-04-12 11:42:30.070095359 +0200
> +++ b/drivers/gpu/drm/Kconfig 2019-04-12 11:42:30.066095359 +0200
> @@ -37,7 +37,6 @@ config DRM_DP_AUX_CHARDEV
>
> config DRM_DEBUG_MM
> bool "Insert extra checks and debug info into the DRM range managers"
> - default n
> depends on DRM=y
> depends on STACKTRACE_SUPPORT
> select STACKDEPOT
> @@ -56,7 +55,6 @@ config DRM_DEBUG_SELFTEST
> select PRIME_NUMBERS
> select DRM_LIB_RANDOM
> select DRM_KMS_HELPER
> - default n
> help
> This option provides kernel modules that can be used to run
> various selftests on parts of the DRM api. This option is not
> @@ -113,7 +111,6 @@ config DRM_FBDEV_OVERALLOC
> config DRM_FBDEV_LEAK_PHYS_SMEM
> bool "Shamelessly allow leaking of fbdev physical address (DANGEROUS)"
> depends on DRM_FBDEV_EMULATION && EXPERT
> - default n
> help
> In order to keep user-space compatibility, we want in certain
> use-cases to keep leaking the fbdev physical address to the
> @@ -247,7 +244,6 @@ config DRM_VKMS
> tristate "Virtual KMS (EXPERIMENTAL)"
> depends on DRM
> select DRM_KMS_HELPER
> - default n
> help
> Virtual Kernel Mode-Setting (VKMS) is used for testing or for
> running GPU in a headless machines. Choose this option to get
> @@ -424,4 +420,3 @@ config DRM_PANEL_ORIENTATION_QUIRKS
>
> config DRM_LIB_RANDOM
> bool
> - default n
> Index: b/drivers/gpu/drm/amd/amdgpu/Kconfig
> ===
> --- a/drivers/gpu/drm/amd/amdgpu/Kconfig 2019-04-12 11:42:30.070095359
> +0200
> +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig 2019-04-12 11:42:30.066095359
> +0200
> @@ -35,7 +35,6 @@ config DRM_AMDGPU_GART_DEBUGFS
> bool "Allow GART access through debugfs"
> depends on DRM_AMDGPU
> depends on DEBUG_FS
> - default n
> help
> Selecting this option creates a debugfs file to inspect the mapped
> pages. Uses more memory for housekeeping, enable only for debugging.
> Index: b/drivers/gpu/drm/arm/Kconfig
> ===
> --- a/drivers/gpu/drm/arm/Kconfig 2019-04-12 11:42:30.070095359 +0200
> +++ b/drivers/gpu/drm/arm/Kconfig 2019-04-12 11:42:30.066095359 +0200
> @@ -16,7 +16,6 @@ config DRM_HDLCD
> config DRM_HDLCD_SHOW_UNDERRUN
> bool "Show underrun conditions"
> depends on DRM_HDLCD
> - default n
> help
> Enable this option to show in red colour the pixels that the
> HDLCD device did not fetch from framebuffer due to underrun
> Index: b/drivers/gpu/drm/exynos/Kconfig
> ===
> --- a/drivers/gpu/drm/exynos/Kconfig 2019-04-12 11:42:30.070095359 +0200
> +++ b/
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
Re: [Freedreno] [PATCH] drm: Split out drm_probe_helper.h
On Tue, 15 Jan 2019, Daniel Vetter wrote: > Having the probe helper stuff (which pretty much everyone needs) in > the drm_crtc_helper.h file (which atomic drivers should never need) is > confusing. Split them out. > > To make sure I actually achieved the goal here I went through all > drivers. And indeed, all atomic drivers are now free of > drm_crtc_helper.h includes. > > v2: Make it compile. There was so much compile fail on arm drivers > that I figured I'll better not include any of the acks on v1. > > v3: Massive rebase because i915 has lost a lot of drmP.h includes, but > not all: Through drm_crtc_helper.h > drm_modeset_helper.h -> drmP.h > there was still one, which this patch largely removes. Which means > rolling out lots more includes all over. > > This will also conflict with ongoing drmP.h cleanup by others I > expect. > > v3: Rebase on top of atomic bochs. > > Cc: Sam Ravnborg > Cc: Jani Nikula > Cc: Laurent Pinchart > Acked-by: Rodrigo Vivi (v2) > Acked-by: Benjamin Gaignard (v2) > Signed-off-by: Daniel Vetter > Cc: linux-arm-ker...@lists.infradead.org > Cc: virtualizat...@lists.linux-foundation.org > Cc: etna...@lists.freedesktop.org > Cc: linux-samsung-...@vger.kernel.org > Cc: intel-...@lists.freedesktop.org > Cc: linux-media...@lists.infradead.org > Cc: linux-amlo...@lists.infradead.org > Cc: linux-arm-...@vger.kernel.org > Cc: freedreno@lists.freedesktop.org > Cc: nouv...@lists.freedesktop.org > Cc: spice-de...@lists.freedesktop.org > Cc: amd-...@lists.freedesktop.org > Cc: linux-renesas-...@vger.kernel.org > Cc: linux-rockc...@lists.infradead.org > Cc: linux-st...@st-md-mailman.stormreply.com > Cc: linux-te...@vger.kernel.org > Cc: xen-de...@lists.xen.org > --- > Merging this is going to be a bit a mess due to all the ongoing drmP.h > cleanups. I think the following should work: > - Apply Sam's prep patches for removing drmP.h from > drm_modeset_helper.h > - Get the i915 drmP.h cleanup backmerged into drm-misc-next > - Apply this patch. > - Apply Sam's patch to remove drmP.h from drm_modeset_helper.h > - All through drm-misc-next, which has some potential for trivial > conflicts around #includes with other drivers unfortunately. > > I hope there's no other driver who'll blow up accidentally because > someone else is doing a drmP.h cleanup. Laurent maybe? > > Jani, ack on this? Acked-by: Jani Nikula -- Jani Nikula, Intel Open Source Graphics Center ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Tue, 20 Nov 2018, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Make life easier for drivers by simply passing the connector
> to drm_hdmi_avi_infoframe_from_display_mode() and
> drm_hdmi_avi_infoframe_quant_range(). That way drivers don't
> need to worry about is_hdmi2_sink mess.
Overall looks about right and nice,
Reviewed-by: Jani Nikula
But please do take that with a grain of salt for everything outside of
i915 and drm core.
Please also find a few comments inline below.
> Cc: Alex Deucher
> Cc: "Christian König"
> Cc: "David (ChunMing) Zhou"
> Cc: Archit Taneja
> Cc: Andrzej Hajda
> Cc: Laurent Pinchart
> Cc: Inki Dae
> Cc: Joonyoung Shim
> Cc: Seung-Woo Kim
> Cc: Kyungmin Park
> Cc: Russell King
> Cc: CK Hu
> Cc: Philipp Zabel
> Cc: Rob Clark
> Cc: Ben Skeggs
> Cc: Tomi Valkeinen
> Cc: Sandy Huang
> Cc: "Heiko Stübner"
> Cc: Benjamin Gaignard
> Cc: Vincent Abriou
> Cc: Thierry Reding
> Cc: Eric Anholt
> Cc: Shawn Guo
> Cc: Ilia Mirkin
> Cc: amd-...@lists.freedesktop.org
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> Cc: nouv...@lists.freedesktop.org
> Cc: linux-te...@vger.kernel.org
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +-
> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +-
> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++-
> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +-
> drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++--
> drivers/gpu/drm/bridge/sii902x.c | 3 ++-
> drivers/gpu/drm/bridge/sil-sii8620.c | 3 +--
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++-
> drivers/gpu/drm/drm_edid.c| 33 ++-
> drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++-
> drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++-
> drivers/gpu/drm/i915/intel_hdmi.c | 14 +-
> drivers/gpu/drm/i915/intel_lspcon.c | 15 ++-
> drivers/gpu/drm/i915/intel_sdvo.c | 10 ---
> drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 ++-
> drivers/gpu/drm/msm/hdmi/hdmi_bridge.c| 3 ++-
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 7 +++--
> drivers/gpu/drm/omapdrm/omap_encoder.c| 5 ++--
> drivers/gpu/drm/radeon/radeon_audio.c | 2 +-
> drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++-
> drivers/gpu/drm/sti/sti_hdmi.c| 3 ++-
> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c| 3 ++-
> drivers/gpu/drm/tegra/hdmi.c | 3 ++-
> drivers/gpu/drm/tegra/sor.c | 3 ++-
> drivers/gpu/drm/vc4/vc4_hdmi.c| 11 +---
> drivers/gpu/drm/zte/zx_hdmi.c | 4 ++-
> include/drm/drm_edid.h| 8 +++---
> 27 files changed, 94 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> index 4cfecdce29a3..1f0426d2fc2a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -1682,7 +1682,7 @@ static void dce_v10_0_afmt_setmode(struct drm_encoder
> *encoder,
> dce_v10_0_audio_write_sad_regs(encoder);
> dce_v10_0_audio_write_latency_fields(encoder, mode);
>
> - err = drm_hdmi_avi_infoframe_from_display_mode(, mode, false);
> + err = drm_hdmi_avi_infoframe_from_display_mode(, connector, mode);
> if (err < 0) {
> DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
> return;
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> index 7c868916d90f..2280b971d758 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -1724,7 +1724,7 @@ static void dce_v11_0_afmt_setmode(struct drm_encoder
> *encoder,
> dce_v11_0_audio_write_sad_regs(encoder);
> dce_v11_0_audio_write_latency_fields(encoder, mode);
>
> - err = drm_hdmi_avi_infoframe_from_display_mode(, mode, false);
> + err = drm_hdmi_avi_infoframe_from_display_mode(, connector, mode);
> if (err < 0) {
> DRM_ERROR("failed to setup AVI infoframe: %zd\n", err);
> return;
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> index 17eaaba36017..db443ec53d3a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> @@ -1423,6 +1423,7 @@ static void dce_v6_0_audio_set_avi_infoframe(struct
> drm_encoder *encoder,
> struct amdgpu_device *adev = dev->dev_private;
> struct amdgpu_encoder *amdgpu_encoder = to
Re: [Freedreno] [PATCH] gpu: Consistently use octal not symbolic permissions
On Thu, 24 May 2018, Joe Perches <j...@perches.com> wrote: > On Fri, 2018-05-25 at 09:41 +0300, Jani Nikula wrote: >> On Thu, 24 May 2018, Joe Perches <j...@perches.com> wrote: >> > There is currently a mixture of octal and symbolic permissions uses >> > in files in drivers/gpu/drm and one file in drivers/gpu. >> > >> > There are ~270 existing octal uses and ~115 S_ uses. >> > >> > Convert all the S_ symbolic permissions to their octal equivalents >> > as using octal and not symbolic permissions is preferred by many as more >> > readable. >> > >> > see: https://lkml.org/lkml/2016/8/2/1945 >> > >> > Done with automated conversion via: >> > $ ./scripts/checkpatch.pl -f --types=SYMBOLIC_PERMS --fix-inplace >> > >> > >> > Miscellanea: >> > >> > o Wrapped modified multi-line calls to a single line where appropriate >> > o Realign modified multi-line calls to open parenthesis >> > o drivers/gpu/drm/msm/adreno/a5xx_debugfs.c has a world-writeable >> > debug permission for "reset" - perhaps that should be modified >> > Signed-off-by: Joe Perches <j...@perches.com> >> > --- >> > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c| 2 +- >> > drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 98 >> > +++--- >> > drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 3 +- >> > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 9 +- >> > drivers/gpu/drm/armada/armada_debugfs.c| 4 +- >> > drivers/gpu/drm/drm_debugfs.c | 6 +- >> > drivers/gpu/drm/drm_debugfs_crc.c | 4 +- >> > drivers/gpu/drm/drm_sysfs.c| 2 +- >> > drivers/gpu/drm/i915/gvt/firmware.c| 2 +- >> > drivers/gpu/drm/i915/i915_debugfs.c| 8 +- >> > drivers/gpu/drm/i915/i915_perf.c | 2 +- >> > drivers/gpu/drm/i915/i915_sysfs.c | 22 ++--- >> > drivers/gpu/drm/i915/intel_pipe_crc.c | 2 +- >> >> Please send at least i915 changes separately. There's zero reason to >> make our lives harder for this change. > > The idea is to avoid unnecessary multiple patches for > individual trees. You're changing like a dozen trees. And apparently maintainers of different trees also have different opinions on whether this is a good idea or not. Mass changes like this go nowhere, and if they do, will cause unnecessary conflicts that could have been trivially avoided. BR, Jani. > But you could do that via something like: > > $ git am --include='drivers/gpu/drm/i915/*' > > cheers, Joe > -- Jani Nikula, Intel Open Source Graphics Center ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH] gpu: Consistently use octal not symbolic permissions
On Thu, 24 May 2018, Joe Perches <j...@perches.com> wrote: > There is currently a mixture of octal and symbolic permissions uses > in files in drivers/gpu/drm and one file in drivers/gpu. > > There are ~270 existing octal uses and ~115 S_ uses. > > Convert all the S_ symbolic permissions to their octal equivalents > as using octal and not symbolic permissions is preferred by many as more > readable. > > see: https://lkml.org/lkml/2016/8/2/1945 > > Done with automated conversion via: > $ ./scripts/checkpatch.pl -f --types=SYMBOLIC_PERMS --fix-inplace > > Miscellanea: > > o Wrapped modified multi-line calls to a single line where appropriate > o Realign modified multi-line calls to open parenthesis > o drivers/gpu/drm/msm/adreno/a5xx_debugfs.c has a world-writeable > debug permission for "reset" - perhaps that should be modified > > Signed-off-by: Joe Perches <j...@perches.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c| 2 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 98 > +++--- > drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 3 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 9 +- > drivers/gpu/drm/armada/armada_debugfs.c| 4 +- > drivers/gpu/drm/drm_debugfs.c | 6 +- > drivers/gpu/drm/drm_debugfs_crc.c | 4 +- > drivers/gpu/drm/drm_sysfs.c| 2 +- > drivers/gpu/drm/i915/gvt/firmware.c| 2 +- > drivers/gpu/drm/i915/i915_debugfs.c| 8 +- > drivers/gpu/drm/i915/i915_perf.c | 2 +- > drivers/gpu/drm/i915/i915_sysfs.c | 22 ++--- > drivers/gpu/drm/i915/intel_pipe_crc.c | 2 +- Please send at least i915 changes separately. There's zero reason to make our lives harder for this change. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH] fix double ;;s in code
On Mon, 19 Feb 2018, Pavel Machek <pa...@ucw.cz> wrote: > On Mon 2018-02-19 16:41:35, Daniel Vetter wrote: >> Yeah, pls split this into one patch per area, with a suitable patch >> subject prefix. Look at git log of each file to get a feeling for what's >> the standard in each area. > > Yeah I can spend hour spliting it, and then people will ignore it > anyway. > > If you care about one of the files being modified, please fix the > bug, ";;" is a clear bug. > > If you don't care ... well I don't care either. Look, if this causes just one conflict down the line because it touches the kernel all over the place, then IMO it already wasn't worth it. Merge conflicts are inevitable, but there's no reason to make life harder just to cater for a cleanup patch. It's not that important. Had it been split up, the drm parts would've been merged already. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [patch v2] drm/msm/dsi: free first element on error
On Sun, 26 Feb 2017, Daniel Vetter <dan...@ffwll.ch> wrote:
> On Thu, Feb 16, 2017 at 02:27:08PM +0200, Jani Nikula wrote:
>> On Thu, 16 Feb 2017, Rob Clark <robdcl...@gmail.com> wrote:
>> > On Thu, Feb 16, 2017 at 7:00 AM, Dan Carpenter <dan.carpen...@oracle.com>
>> > wrote:
>> >> We're off by one here. We free something that wasn't allocated and we
>> >> don't free msm_host->bus_clks[].
>> >>
>> >> Fixes: 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")
>> >> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
>> >
>> > Thanks
>> >
>> > Reviewed-by: Rob Clark <robdcl...@gmail.com>
>>
>> And for good measure,
>>
>> Reviewed-by: Jani Nikula <jani.nik...@intel.com>
>
> So 2 r-b from maintainers, no one said he'll apply. This isn't really
> great coordination :-) Note that drm-misc-next is always open, so you
> could always smash it in there, irrespective of merge window state. hint,
> hint ...
Admittedly I shied away from touching drm/msm.
BR,
Jani.
> -Daniel
>
>>
>>
>> >
>> >>
>> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> >> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> >> index 1fc07ce24686..4fa32296162e 100644
>> >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> >> @@ -437,7 +437,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host
>> >> *msm_host)
>> >>
>> >> return 0;
>> >> err:
>> >> - for (; i > 0; i--)
>> >> + while (i--)
>> >> clk_disable_unprepare(msm_host->bus_clks[i]);
>> >>
>> >> return ret;
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
>> ___
>> dri-devel mailing list
>> dri-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Jani Nikula, Intel Open Source Technology Center
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
