Re: [PATCH v3 2/9] of: property: add of_graph_get_next_port_endpoint()
Hi Rob, On Tue, Aug 27, 2024 at 09:05:02AM -0500, Rob Herring wrote: > On Tue, Aug 27, 2024 at 5:47 AM Sakari Ailus wrote: > > > > Rob, Kunimori-san, > > > > On Mon, Aug 26, 2024 at 10:40:09AM -0500, Rob Herring wrote: > > > On Mon, Aug 26, 2024 at 02:43:28AM +, Kuninori Morimoto wrote: > > > > We already have of_graph_get_next_endpoint(), but it is not > > > > intuitive to use in some case. > > > > > > Can of_graph_get_next_endpoint() users be replaced with your new > > > helpers? I'd really like to get rid of the 3 remaining users. > > > > The fwnode graph API has fwnode_graph_get_endpoint_by_id() which can also > > be used to obtain endpoints within a port. It does the same than > > of_graph_get_endpoint_by_regs() with the addition that it also has a > > flags field to allow e.g. returning endpoints with regs higher than > > requested (FWNODE_GRAPH_ENDPOINT_NEXT). > > Looks to me like FWNODE_GRAPH_ENDPOINT_NEXT is always used with > endpoint #0. That's equivalent to passing -1 for the endpoint number > with the OF API. If the caller needs a single endpoint only, then the two are the same, yes. The NEXT flag can still be used for obtaining further endpoints, unlike setting endpoint to -1 in of_graph_get_endpoint_by_regs(). > > > Most users dealing with endpoints on fwnode property API use this, could > > something like this be done on OF as well? Probably a similar flag would be > > needed though. > > I had fixed almost all the OF cases at one point. Unfortunately, there > were a few corner cases that I didn't address to eliminate the API. So > now it has proliferated with the fwnode API. Much of the usage of fwnode_graph_get_next_endpoint() is conversion from the OF API but there are some newer drivers, too. I admit I've sometimes missed this in review. At the same time I can say most users in the media tree do employ fwnode_graph_get_endpoint_by_id() already. The good thing is that almost all current users are camera sensors and converting them is fairly trivial. I can post patches but it'll take a while... -- Kind regards, Sakari Ailus
Re: [PATCH v3 2/9] of: property: add of_graph_get_next_port_endpoint()
Rob, Kunimori-san, On Mon, Aug 26, 2024 at 10:40:09AM -0500, Rob Herring wrote: > On Mon, Aug 26, 2024 at 02:43:28AM +, Kuninori Morimoto wrote: > > We already have of_graph_get_next_endpoint(), but it is not > > intuitive to use in some case. > > Can of_graph_get_next_endpoint() users be replaced with your new > helpers? I'd really like to get rid of the 3 remaining users. The fwnode graph API has fwnode_graph_get_endpoint_by_id() which can also be used to obtain endpoints within a port. It does the same than of_graph_get_endpoint_by_regs() with the addition that it also has a flags field to allow e.g. returning endpoints with regs higher than requested (FWNODE_GRAPH_ENDPOINT_NEXT). Most users dealing with endpoints on fwnode property API use this, could something like this be done on OF as well? Probably a similar flag would be needed though. -- Kind regards, Sakari Ailus
Re: [PATCH v2 1/9] of: property: add of_graph_get_next_port()
gt; > +
> > +/**
> > + * for_each_of_graph_port - iterate over every port in a device or ports
> > node
> > + * @parent: parent device or ports node containing port
> > + * @child: loop variable pointing to the current port node
> > + *
> > + * When breaking out of the loop, of_node_put(child) has to be called
> > manually.
> > + */
> > +#define for_each_of_graph_port(parent, child) \
> > + for (child = of_graph_get_next_port(parent, NULL); child != NULL; \
> > +child = of_graph_get_next_port(parent, child))
>
> I think I've proposed something similar a long time ago, and was
> told that iterating over ports is not something that drivers should do.
> The situation may have changed since though.
>
> Sakari, any opinion on this ?
It'd be good to understand first what would be the use case for it. There
is already a function to obtain endpoints within a given port, including an
fwnode equivalent.
>
> > +
> > #ifdef CONFIG_OF
> > bool of_graph_is_present(const struct device_node *node);
> > int of_graph_parse_endpoint(const struct device_node *node,
> > struct of_endpoint *endpoint);
> > unsigned int of_graph_get_endpoint_count(const struct device_node *np);
> > +unsigned int of_graph_get_port_count(struct device_node *np);
> > struct device_node *of_graph_get_port_by_id(struct device_node *node, u32
> > id);
> > struct device_node *of_graph_get_next_endpoint(const struct device_node
> > *parent,
> > struct device_node *previous);
> > +struct device_node *of_graph_get_next_ports(struct device_node *parent,
> > + struct device_node *ports);
> > +struct device_node *of_graph_get_next_port(struct device_node *parent,
> > + struct device_node *port);
> > struct device_node *of_graph_get_endpoint_by_regs(
> > const struct device_node *parent, int port_reg, int reg);
> > struct device_node *of_graph_get_remote_endpoint(
> > @@ -73,6 +100,11 @@ static inline unsigned int
> > of_graph_get_endpoint_count(const struct device_node
> > return 0;
> > }
> >
> > +static inline unsigned int of_graph_get_port_count(struct device_node *np)
> > +{
> > + return 0;
> > +}
> > +
> > static inline struct device_node *of_graph_get_port_by_id(
> > struct device_node *node, u32 id)
> > {
> > @@ -86,6 +118,20 @@ static inline struct device_node
> > *of_graph_get_next_endpoint(
> > return NULL;
> > }
> >
> > +static inline struct device_node *of_graph_get_next_ports(
> > + struct device_node *parent,
> > + struct device_node *previous)
> > +{
> > + return NULL;
> > +}
> > +
> > +static inline struct device_node *of_graph_get_next_port(
> > + struct device_node *parent,
> > + struct device_node *previous)
> > +{
> > + return NULL;
> > +}
> > +
> > static inline struct device_node *of_graph_get_endpoint_by_regs(
> > const struct device_node *parent, int port_reg, int reg)
> > {
>
--
Kind regards,
Sakari Ailus
Re: [PATCH v1 1/1] treewide: Align match_string() with sysfs_match_string()
Hi Andy,
On Sun, Jun 02, 2024 at 06:57:12PM +0300, Andy Shevchenko wrote:
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 503773707e01..9cb350de30f0 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -798,7 +798,7 @@ static bool acpi_info_matches_ids(struct acpi_device_info
> *info,
> if (!(info->valid & ACPI_VALID_HID))
> return false;
>
> - index = match_string(ids, -1, info->hardware_id.string);
> + index = __match_string(ids, -1, info->hardware_id.string);
> if (index >= 0)
> return true;
>
> @@ -809,7 +809,7 @@ static bool acpi_info_matches_ids(struct acpi_device_info
> *info,
> return false;
>
> for (i = 0; i < cid_list->count; i++) {
> - index = match_string(ids, -1, cid_list->ids[i].string);
> + index = __match_string(ids, -1, cid_list->ids[i].string);
> if (index >= 0)
> return true;
> }
Reviewed-by: Sakari Ailus # drivers/acpi
--
Sakari Ailus
Re: [PATCH 0/3] kci-gitlab: Introducing GitLab-CI Pipeline for Kernel Testing
Hi Laurent, Helen, On Thu, Feb 29, 2024 at 01:07:25AM +0200, Laurent Pinchart wrote: > > **Join Our Slack Channel:** > > We have a Slack channel, #gitlab-ci, on the KernelCI Slack instance > > https://kernelci.slack.com/ . > > Feel free to join and contribute to the conversation. The KernelCI team has > > weekly calls where we also discuss the GitLab-CI pipeline. > > Could we communicate using free software please ? Furthermore, it's not > possible to create an account on that slack instance unless you have an > e-mail address affiliated with a small number of companies > (https://kernelci.slack.com/signup#/domain-signup). That's a big no-go > for me. I agree. There is no shortage of good alternatives either: we have IRC, Matrix and XMPP for instance. Just pick one of them. -- Regards, Sakari Ailus
[PATCH v5 1/2] PM: runtime: Simplify pm_runtime_get_if_active() usage
There are two ways to opportunistically increment a device's runtime PM
usage count, calling either pm_runtime_get_if_active() or
pm_runtime_get_if_in_use(). The former has an argument to tell whether to
ignore the usage count or not, and the latter simply calls the former with
ign_usage_count set to false. The other users that want to ignore the
usage_count will have to explicitly set that argument to true which is a
bit cumbersome.
To make this function more practical to use, remove the ign_usage_count
argument from the function. The main implementation is in a static
function called pm_runtime_get_conditional() and implementations of
pm_runtime_get_if_active() and pm_runtime_get_if_in_use() are moved to
runtime.c.
Signed-off-by: Sakari Ailus
Reviewed-by: Alex Elder
Reviewed-by: Laurent Pinchart
Acked-by: Takashi Iwai # sound/
Reviewed-by: Jacek Lawrynowicz #
drivers/accel/ivpu/
Acked-by: Rodrigo Vivi # drivers/gpu/drm/i915/
Reviewed-by: Rodrigo Vivi
Acked-by: Bjorn Helgaas # drivers/pci/
---
Documentation/power/runtime_pm.rst | 5 ++--
drivers/accel/ivpu/ivpu_pm.c| 2 +-
drivers/base/power/runtime.c| 35 +++--
drivers/gpu/drm/i915/intel_runtime_pm.c | 5 +++-
drivers/gpu/drm/xe/xe_pm.c | 2 +-
drivers/media/i2c/ccs/ccs-core.c| 2 +-
drivers/media/i2c/ov64a40.c | 2 +-
drivers/media/i2c/thp7312.c | 2 +-
drivers/net/ipa/ipa_smp2p.c | 2 +-
drivers/pci/pci.c | 2 +-
include/linux/pm_runtime.h | 18 +++--
sound/hda/hdac_device.c | 2 +-
12 files changed, 50 insertions(+), 29 deletions(-)
diff --git a/Documentation/power/runtime_pm.rst
b/Documentation/power/runtime_pm.rst
index 65b86e487afe..da99379071a4 100644
--- a/Documentation/power/runtime_pm.rst
+++ b/Documentation/power/runtime_pm.rst
@@ -396,10 +396,9 @@ drivers/base/power/runtime.c and
include/linux/pm_runtime.h:
nonzero, increment the counter and return 1; otherwise return 0 without
changing the counter
- `int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count);`
+ `int pm_runtime_get_if_active(struct device *dev);`
- return -EINVAL if 'power.disable_depth' is nonzero; otherwise, if the
- runtime PM status is RPM_ACTIVE, and either ign_usage_count is true
- or the device's usage_count is non-zero, increment the counter and
+ runtime PM status is RPM_ACTIVE, increment the counter and
return 1; otherwise return 0 without changing the counter
`void pm_runtime_put_noidle(struct device *dev);`
diff --git a/drivers/accel/ivpu/ivpu_pm.c b/drivers/accel/ivpu/ivpu_pm.c
index 0af8864cb3b5..c6d93c7a1c58 100644
--- a/drivers/accel/ivpu/ivpu_pm.c
+++ b/drivers/accel/ivpu/ivpu_pm.c
@@ -292,7 +292,7 @@ int ivpu_rpm_get_if_active(struct ivpu_device *vdev)
{
int ret;
- ret = pm_runtime_get_if_active(vdev->drm.dev, false);
+ ret = pm_runtime_get_if_in_use(vdev->drm.dev);
drm_WARN_ON(&vdev->drm, ret < 0);
return ret;
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 05793c9fbb84..5275a6b2e980 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1176,7 +1176,7 @@ int __pm_runtime_resume(struct device *dev, int rpmflags)
EXPORT_SYMBOL_GPL(__pm_runtime_resume);
/**
- * pm_runtime_get_if_active - Conditionally bump up device usage counter.
+ * pm_runtime_get_conditional - Conditionally bump up device usage counter.
* @dev: Device to handle.
* @ign_usage_count: Whether or not to look at the current usage counter value.
*
@@ -1197,7 +1197,7 @@ EXPORT_SYMBOL_GPL(__pm_runtime_resume);
* The caller is responsible for decrementing the runtime PM usage counter of
* @dev after this function has returned a positive value for it.
*/
-int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count)
+static int pm_runtime_get_conditional(struct device *dev, bool ign_usage_count)
{
unsigned long flags;
int retval;
@@ -1218,8 +1218,39 @@ int pm_runtime_get_if_active(struct device *dev, bool
ign_usage_count)
return retval;
}
+
+/**
+ * pm_runtime_get_if_active - Bump up runtime PM usage counter if the device is
+ * in active state
+ * @dev: Target device.
+ *
+ * Increment the runtime PM usage counter of @dev if its runtime PM status is
+ * %RPM_ACTIVE, in which case it returns 1. If the device is in a different
+ * state, 0 is returned. -EINVAL is returned if runtime PM is disabled for the
+ * device, in which case also the usage_count will remain unmodified.
+ */
+int pm_runtime_get_if_active(struct device *dev)
+{
+ return pm_runtime_get_conditional(dev, true);
+}
EXPORT_SYMBOL_GPL(pm_runtime_get_if_active);
+/**
+ * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
+ * @dev: Target devi
[PATCH v5 0/2] Small runtime PM API changes
Hi folks,
Here's a small but a different set of patches for making two relatively
minor changes to runtime PM API. I restarted version numbering as this is
meaningfully different from the previous set.
pm_runtime_get_if_active() loses its second argument as it only made sense
to have ign_usage_count argument true.
The other change is also small but it has an effect on callers:
pm_runtime_put_autosuspend() will, in the future, be re-purposed to
include a call to pm_runtime_mark_last_busy() as well. Before this,
current users of the function are moved to __pm_runtime_put_autosuspend()
(added by this patchset) which will continue to have the current
behaviour.
I haven't included the conversion patches in this set as I only want to do
that once this set has been approved and merged. The tree specific patches
can be found here, on linux-next master (there are some V4L2 patches
there, too, please ignore them for now):
https://git.kernel.org/pub/scm/linux/kernel/git/sailus/linux-next.git/log/?
Later on, users calling pm_runtime_mark_last_busy() immediately followed
by __pm_runtime_put_autosuspend() will be switched back to
pm_runtime_put_autosuspend() once its behaviour change has been done (a
patch near top of that branch). I'll provide these once the preceding ones
have been merged.
Comments are welcome.
since v4:
- patch 1: Squash patch 2
https://lists.freedesktop.org/archives/dri-devel/2024-January/438739.html>.
- patch 1: Add to documentation of pm_runtime_get_if_{active,in_use} that
if PM runtime is disabled, an error is returned and the usage_count
remains unmodified.
- patch 1: Fix spelling of "explicitly" in the commit message.
- Use upper case "PM:" prefix in subject.
since v3:
- patch 1: Drop the previously added documentation on driver use of
pm_runtime_get_conditional().
- Add a patch to make pm_runtime_get_conditional() static, including
switching i915 to pm_runtime_get_if_{active,in_use}.
since v2:
- Rebase on v6.8-rc1 (no changes).
- Add Rodrigo's Reviewed-by: to the 1st patch.
since v1:
- patch 1: Rename __pm_runtime_get_conditional() as
pm_runtime_get_conditional().
- patch 1: Reword documentation on driver use of
pm_runtime_get_conditional().
Sakari Ailus (2):
PM: runtime: Simplify pm_runtime_get_if_active() usage
PM: runtime: Add pm_runtime_put_autosuspend() replacement
Documentation/power/runtime_pm.rst | 22 +---
drivers/accel/ivpu/ivpu_pm.c| 2 +-
drivers/base/power/runtime.c| 35 +++--
drivers/gpu/drm/i915/intel_runtime_pm.c | 5 +++-
drivers/gpu/drm/xe/xe_pm.c | 2 +-
drivers/media/i2c/ccs/ccs-core.c| 2 +-
drivers/media/i2c/ov64a40.c | 2 +-
drivers/media/i2c/thp7312.c | 2 +-
drivers/net/ipa/ipa_smp2p.c | 2 +-
drivers/pci/pci.c | 2 +-
include/linux/pm_runtime.h | 30 ++---
sound/hda/hdac_device.c | 2 +-
12 files changed, 73 insertions(+), 35 deletions(-)
--
2.39.2
Re: [PATCH v3 1/2] pm: runtime: Simplify pm_runtime_get_if_active() usage
Hi Alex, On Fri, Jan 26, 2024 at 09:12:02AM -0600, Alex Elder wrote: > On 1/22/24 5:41 AM, Sakari Ailus wrote: > > There are two ways to opportunistically increment a device's runtime PM > > usage count, calling either pm_runtime_get_if_active() or > > pm_runtime_get_if_in_use(). The former has an argument to tell whether to > > ignore the usage count or not, and the latter simply calls the former with > > ign_usage_count set to false. The other users that want to ignore the > > usage_count will have to explitly set that argument to true which is a bit > > cumbersome. > > > > To make this function more practical to use, remove the ign_usage_count > > argument from the function. The main implementation is renamed as > > pm_runtime_get_conditional(). > > > > Signed-off-by: Sakari Ailus > > Reviewed-by: Alex Elder # drivers/net/ipa/ipa_smp2p.c > > I actually intended my "Reviewed-by" to cover the entire patch. I > checked every caller and they all looked good to me. Thanks, I'll drop the file name. AFAIR it was just below that file, so I added it, but I could be wrong, too. v5 will also squash the 2nd patch of v4 into this one https://lore.kernel.org/linux-pm/ZbBAWROxRKE8Y8VU@kekkonen.localdomain/T/#m76d34e679e12d8536a20eb29af6e826e2a85a24b>, I hope that's fine. -- Kind regards, Sakari Ailus
Re: [PATCH v4 1/3] pm: runtime: Simplify pm_runtime_get_if_active() usage
On Tue, Jan 23, 2024 at 03:48:01PM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 23, 2024 at 08:44:04PM +0000, Sakari Ailus wrote:
> > On Tue, Jan 23, 2024 at 11:24:23AM -0600, Bjorn Helgaas wrote:
> > ...
>
> > > - I don't know whether it's feasible, but it would be nice if the
> > > intel_pm_runtime_pm.c rework could be done in one shot instead of
> > > being split between patches 1/3 and 2/3.
> > >
> > > Maybe it could be a preliminary patch that uses the existing
> > > if_active/if_in_use interfaces, followed by the trivial if_active
> > > updates in this patch. I think that would make the history easier
> > > to read than having the transitory pm_runtime_get_conditional() in
> > > the middle.
> >
> > I think I'd merge the two patches. The second patch is fairly small, after
> > all, and both deal with largely the same code.
>
> I'm not sure which two patches you mean, but the fact that two patches
> deal with largely the same code is not necessarily an argument for
> merging them. From a reviewing perspective, it's nice if a patch like
Patches 1 and 2. The third patch introduces a new Runtime PM API function.
> 1/3, where it's largely mechanical and easy to review, is separated
> from patches that make more substantive changes.
>
> That's why I think it'd be nice if the "interesting"
> intel_pm_runtime_pm.c changes were all in the same patch, and ideally,
> if that patch *only* touched intel_pm_runtime_pm.c.
I don't think squashing the second patch to the first really changes this
meaningfully: the i915 driver simply needs both
pm_runtime_get_if_{active,in_use}, and this is what the patch does to other
drivers already. Making the pm_runtime_get_conditional static would also
fit for the first patch if the desire is to not to introduce it at all.
--
Sakari Ailus
Re: [PATCH v4 1/3] pm: runtime: Simplify pm_runtime_get_if_active() usage
Hi Bjorn, Thanks for the review. On Tue, Jan 23, 2024 at 11:24:23AM -0600, Bjorn Helgaas wrote: > On Tue, Jan 23, 2024 at 11:56:42AM +0200, Sakari Ailus wrote: > > There are two ways to opportunistically increment a device's runtime PM > > usage count, calling either pm_runtime_get_if_active() or > > pm_runtime_get_if_in_use(). The former has an argument to tell whether to > > ignore the usage count or not, and the latter simply calls the former with > > ign_usage_count set to false. The other users that want to ignore the > > usage_count will have to explitly set that argument to true which is a bit > > cumbersome. > > > > To make this function more practical to use, remove the ign_usage_count > > argument from the function. The main implementation is renamed as > > pm_runtime_get_conditional(). > > > > Signed-off-by: Sakari Ailus > > Reviewed-by: Alex Elder # drivers/net/ipa/ipa_smp2p.c > > Reviewed-by: Laurent Pinchart > > Acked-by: Takashi Iwai # sound/ > > Reviewed-by: Jacek Lawrynowicz # > > drivers/accel/ivpu/ > > Acked-by: Rodrigo Vivi # drivers/gpu/drm/i915/ > > Reviewed-by: Rodrigo Vivi > > Acked-by: Bjorn Helgaas # drivers/pci/ > > - Previous PM history uses "PM: " in the subject lines (not "pm: "). Oops. I'm not sure why I used lower case. (Maybe I've written too many times "media:" prefix to the subject?) I'll fix this in v5. > > - I don't know whether it's feasible, but it would be nice if the > intel_pm_runtime_pm.c rework could be done in one shot instead of > being split between patches 1/3 and 2/3. > > Maybe it could be a preliminary patch that uses the existing > if_active/if_in_use interfaces, followed by the trivial if_active > updates in this patch. I think that would make the history easier > to read than having the transitory pm_runtime_get_conditional() in > the middle. I think I'd merge the two patches. The second patch is fairly small, after all, and both deal with largely the same code. > > - Similarly, it would be nice if pm_runtime_get_conditional() never > had to be published in pm_runtime.h, instead of being temporarily > added there by this patch and then immediately made private by 2/3. > Maybe that's not practical, I dunno. -- Regards, Sakari Ailus
[PATCH v4 2/3] pm: runtime: Make pm_runtime_get_if_conditional() private
Stop offering pm_runtime_get_if_conditional() API function for drivers,
instead require them to use pm_runtime_get_if_{active,in_use}. Also
convert the only user, the i915 driver, to use the said functions.
Signed-off-by: Sakari Ailus
---
drivers/base/power/runtime.c| 34 --
drivers/gpu/drm/i915/intel_runtime_pm.c | 5 +++-
include/linux/pm_runtime.h | 38 ++---
3 files changed, 38 insertions(+), 39 deletions(-)
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 83e0cd2a19e7..d2c17e29567d 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1197,7 +1197,7 @@ EXPORT_SYMBOL_GPL(__pm_runtime_resume);
* The caller is responsible for decrementing the runtime PM usage counter of
* @dev after this function has returned a positive value for it.
*/
-int pm_runtime_get_conditional(struct device *dev, bool ign_usage_count)
+static int pm_runtime_get_conditional(struct device *dev, bool ign_usage_count)
{
unsigned long flags;
int retval;
@@ -1218,7 +1218,37 @@ int pm_runtime_get_conditional(struct device *dev, bool
ign_usage_count)
return retval;
}
-EXPORT_SYMBOL_GPL(pm_runtime_get_conditional);
+
+/**
+ * pm_runtime_get_if_active - Bump up runtime PM usage counter if the device is
+ * in active state
+ * @dev: Target device.
+ *
+ * Increment the runtime PM usage counter of @dev if its runtime PM status is
+ * %RPM_ACTIVE, in which case it returns 1. If the device is in a different
+ * state, 0 is returned. -EINVAL is returned if runtime PM is disabled for the
+ * device.
+ */
+int pm_runtime_get_if_active(struct device *dev)
+{
+ return pm_runtime_get_conditional(dev, true);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_get_if_active);
+
+/**
+ * pm_runtime_get_if_in_use - Conditionally bump up runtime PM usage counter.
+ * @dev: Target device.
+ *
+ * Increment the runtime PM usage counter of @dev if its runtime PM status is
+ * %RPM_ACTIVE and its runtime PM usage counter is greater than 0, in which
case
+ * it returns 1. If the device is in a different state or its usage_count is 0,
+ * 0 is returned. -EINVAL is returned if runtime PM is disabled for the device.
+ */
+int pm_runtime_get_if_in_use(struct device *dev)
+{
+ return pm_runtime_get_conditional(dev, false);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
/**
* __pm_runtime_set_status - Set runtime PM status of a device.
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index b5f8abd2a22b..d4e844128826 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -246,7 +246,10 @@ static intel_wakeref_t
__intel_runtime_pm_get_if_active(struct intel_runtime_pm
* function, since the power state is undefined. This applies
* atm to the late/early system suspend/resume handlers.
*/
- if (pm_runtime_get_conditional(rpm->kdev, ignore_usecount) <= 0)
+ if ((ignore_usecount &&
+pm_runtime_get_if_active(rpm->kdev) <= 0) ||
+ (!ignore_usecount &&
+pm_runtime_get_if_in_use(rpm->kdev) <= 0))
return 0;
}
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index a212b3008ade..436baa167498 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -72,8 +72,8 @@ extern int pm_runtime_force_resume(struct device *dev);
extern int __pm_runtime_idle(struct device *dev, int rpmflags);
extern int __pm_runtime_suspend(struct device *dev, int rpmflags);
extern int __pm_runtime_resume(struct device *dev, int rpmflags);
-extern int pm_runtime_get_conditional(struct device *dev,
- bool ign_usage_count);
+extern int pm_runtime_get_if_active(struct device *dev);
+extern int pm_runtime_get_if_in_use(struct device *dev);
extern int pm_schedule_suspend(struct device *dev, unsigned int delay);
extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
extern int pm_runtime_barrier(struct device *dev);
@@ -95,35 +95,6 @@ extern void pm_runtime_release_supplier(struct device_link
*link);
extern int devm_pm_runtime_enable(struct device *dev);
-/**
- * pm_runtime_get_if_active - Bump up runtime PM usage counter if the device is
- * in active state
- * @dev: Target device.
- *
- * Increment the runtime PM usage counter of @dev if its runtime PM status is
- * %RPM_ACTIVE, in which case it returns 1. If the device is in a different
- * state, 0 is returned. -EINVAL is returned if runtime PM is disabled for the
- * device.
- */
-static inline int pm_runtime_get_if_active(struct device *dev)
-{
- return pm_runtime_get_conditional(dev, true);
-}
-
-/**
- * pm_runtime_get_if_in_use - Cond
[PATCH v4 1/3] pm: runtime: Simplify pm_runtime_get_if_active() usage
There are two ways to opportunistically increment a device's runtime PM
usage count, calling either pm_runtime_get_if_active() or
pm_runtime_get_if_in_use(). The former has an argument to tell whether to
ignore the usage count or not, and the latter simply calls the former with
ign_usage_count set to false. The other users that want to ignore the
usage_count will have to explitly set that argument to true which is a bit
cumbersome.
To make this function more practical to use, remove the ign_usage_count
argument from the function. The main implementation is renamed as
pm_runtime_get_conditional().
Signed-off-by: Sakari Ailus
Reviewed-by: Alex Elder # drivers/net/ipa/ipa_smp2p.c
Reviewed-by: Laurent Pinchart
Acked-by: Takashi Iwai # sound/
Reviewed-by: Jacek Lawrynowicz #
drivers/accel/ivpu/
Acked-by: Rodrigo Vivi # drivers/gpu/drm/i915/
Reviewed-by: Rodrigo Vivi
---
Documentation/power/runtime_pm.rst | 5 ++--
drivers/accel/ivpu/ivpu_pm.c| 2 +-
drivers/base/power/runtime.c| 6 ++---
drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
drivers/gpu/drm/xe/xe_pm.c | 2 +-
drivers/media/i2c/ccs/ccs-core.c| 2 +-
drivers/media/i2c/ov64a40.c | 2 +-
drivers/media/i2c/thp7312.c | 2 +-
drivers/net/ipa/ipa_smp2p.c | 2 +-
drivers/pci/pci.c | 2 +-
include/linux/pm_runtime.h | 32 +
sound/hda/hdac_device.c | 2 +-
12 files changed, 41 insertions(+), 20 deletions(-)
diff --git a/Documentation/power/runtime_pm.rst
b/Documentation/power/runtime_pm.rst
index 65b86e487afe..da99379071a4 100644
--- a/Documentation/power/runtime_pm.rst
+++ b/Documentation/power/runtime_pm.rst
@@ -396,10 +396,9 @@ drivers/base/power/runtime.c and
include/linux/pm_runtime.h:
nonzero, increment the counter and return 1; otherwise return 0 without
changing the counter
- `int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count);`
+ `int pm_runtime_get_if_active(struct device *dev);`
- return -EINVAL if 'power.disable_depth' is nonzero; otherwise, if the
- runtime PM status is RPM_ACTIVE, and either ign_usage_count is true
- or the device's usage_count is non-zero, increment the counter and
+ runtime PM status is RPM_ACTIVE, increment the counter and
return 1; otherwise return 0 without changing the counter
`void pm_runtime_put_noidle(struct device *dev);`
diff --git a/drivers/accel/ivpu/ivpu_pm.c b/drivers/accel/ivpu/ivpu_pm.c
index 0af8864cb3b5..c6d93c7a1c58 100644
--- a/drivers/accel/ivpu/ivpu_pm.c
+++ b/drivers/accel/ivpu/ivpu_pm.c
@@ -292,7 +292,7 @@ int ivpu_rpm_get_if_active(struct ivpu_device *vdev)
{
int ret;
- ret = pm_runtime_get_if_active(vdev->drm.dev, false);
+ ret = pm_runtime_get_if_in_use(vdev->drm.dev);
drm_WARN_ON(&vdev->drm, ret < 0);
return ret;
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 05793c9fbb84..83e0cd2a19e7 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1176,7 +1176,7 @@ int __pm_runtime_resume(struct device *dev, int rpmflags)
EXPORT_SYMBOL_GPL(__pm_runtime_resume);
/**
- * pm_runtime_get_if_active - Conditionally bump up device usage counter.
+ * pm_runtime_get_conditional - Conditionally bump up device usage counter.
* @dev: Device to handle.
* @ign_usage_count: Whether or not to look at the current usage counter value.
*
@@ -1197,7 +1197,7 @@ EXPORT_SYMBOL_GPL(__pm_runtime_resume);
* The caller is responsible for decrementing the runtime PM usage counter of
* @dev after this function has returned a positive value for it.
*/
-int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count)
+int pm_runtime_get_conditional(struct device *dev, bool ign_usage_count)
{
unsigned long flags;
int retval;
@@ -1218,7 +1218,7 @@ int pm_runtime_get_if_active(struct device *dev, bool
ign_usage_count)
return retval;
}
-EXPORT_SYMBOL_GPL(pm_runtime_get_if_active);
+EXPORT_SYMBOL_GPL(pm_runtime_get_conditional);
/**
* __pm_runtime_set_status - Set runtime PM status of a device.
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 860b51b56a92..b5f8abd2a22b 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -246,7 +246,7 @@ static intel_wakeref_t
__intel_runtime_pm_get_if_active(struct intel_runtime_pm
* function, since the power state is undefined. This applies
* atm to the late/early system suspend/resume handlers.
*/
- if (pm_runtime_get_if_active(rpm->kdev, ignore_usecount) <= 0)
+ if (pm_runtime_get_conditional(rpm->kdev, ignore_usecount) <= 0)
return 0;
}
diff
[PATCH v4 0/3] Small runtime PM API changes
Hi folks,
Here's a small but a different set of patches for making two relatively
minor changes to runtime PM API. I restarted version numbering as this is
meaningfully different from the previous set.
pm_runtime_get_if_active() loses its second argument as it only made sense
to have ign_usage_count argument true.
The other change is also small but it has an effect on callers:
pm_runtime_put_autosuspend() will, in the future, be re-purposed to
include a call to pm_runtime_mark_last_busy() as well. Before this,
current users of the function are moved to __pm_runtime_put_autosuspend()
(added by this patchset) which will continue to have the current
behaviour.
I haven't included the conversion patches in this set as I only want to do
that once this set has been approved and merged. The tree specific patches
can be found here, on linux-next master (there are some V4L2 patches
there, too, please ignore them for now):
https://git.kernel.org/pub/scm/linux/kernel/git/sailus/linux-next.git/log/?
Later on, users calling pm_runtime_mark_last_busy() immediately followed
by __pm_runtime_put_autosuspend() will be switched back to
pm_runtime_put_autosuspend() once its behaviour change has been done (a
patch near top of that branch). I'll provide these once the preceding ones
have been merged.
Comments are welcome.
since v3:
- patch 1: Drop the previously added documentation on driver use of
pm_runtime_get_conditional().
- Add a patch to make pm_runtime_get_conditional() static, including
switching i915 to pm_runtime_get_if_{active,in_use}.
since v2:
- Rebase on v6.8-rc1 (no changes).
- Add Rodrigo's Reviewed-by: to the 1st patch.
since v1:
- patch 1: Rename __pm_runtime_get_conditional() as
pm_runtime_get_conditional().
- patch 1: Reword documentation on driver use of
pm_runtime_get_conditional().
Sakari Ailus (3):
pm: runtime: Simplify pm_runtime_get_if_active() usage
pm: runtime: Make pm_runtime_get_if_conditional() private
pm: runtime: Add pm_runtime_put_autosuspend() replacement
Documentation/power/runtime_pm.rst | 22 +---
drivers/accel/ivpu/ivpu_pm.c| 2 +-
drivers/base/power/runtime.c| 34 +++--
drivers/gpu/drm/i915/intel_runtime_pm.c | 5 +++-
drivers/gpu/drm/xe/xe_pm.c | 2 +-
drivers/media/i2c/ccs/ccs-core.c| 2 +-
drivers/media/i2c/ov64a40.c | 2 +-
drivers/media/i2c/thp7312.c | 2 +-
drivers/net/ipa/ipa_smp2p.c | 2 +-
drivers/pci/pci.c | 2 +-
include/linux/pm_runtime.h | 30 +++---
sound/hda/hdac_device.c | 2 +-
12 files changed, 72 insertions(+), 35 deletions(-)
--
2.39.2
Re: [PATCH v3 1/2] pm: runtime: Simplify pm_runtime_get_if_active() usage
Hi Rafael, Björn,
Thanks for the review.
On Mon, Jan 22, 2024 at 07:16:54PM +0100, Rafael J. Wysocki wrote:
> On Mon, Jan 22, 2024 at 7:12 PM Bjorn Helgaas wrote:
> >
> > On Mon, Jan 22, 2024 at 01:41:21PM +0200, Sakari Ailus wrote:
> > > There are two ways to opportunistically increment a device's runtime PM
> > > usage count, calling either pm_runtime_get_if_active() or
> > > pm_runtime_get_if_in_use(). The former has an argument to tell whether to
> > > ignore the usage count or not, and the latter simply calls the former with
> > > ign_usage_count set to false. The other users that want to ignore the
> > > usage_count will have to explitly set that argument to true which is a bit
> > > cumbersome.
> >
> > s/explitly/explicitly/
> >
> > > To make this function more practical to use, remove the ign_usage_count
> > > argument from the function. The main implementation is renamed as
> > > pm_runtime_get_conditional().
> > >
> > > Signed-off-by: Sakari Ailus
> > > Reviewed-by: Alex Elder # drivers/net/ipa/ipa_smp2p.c
> > > Reviewed-by: Laurent Pinchart
> > > Acked-by: Takashi Iwai # sound/
> > > Reviewed-by: Jacek Lawrynowicz #
> > > drivers/accel/ivpu/
> > > Acked-by: Rodrigo Vivi # drivers/gpu/drm/i915/
> > > Reviewed-by: Rodrigo Vivi
> >
> > Acked-by: Bjorn Helgaas # drivers/pci/
> >
> > > -EXPORT_SYMBOL_GPL(pm_runtime_get_if_active);
> > > +EXPORT_SYMBOL_GPL(pm_runtime_get_conditional);
> >
> > If pm_runtime_get_conditional() is exported, shouldn't it also be
> > documented in Documentation/power/runtime_pm.rst?
> >
> > But I'm dubious about exporting it because
> > __intel_runtime_pm_get_if_active() is the only caller, and you end up
> > with the same pattern there that we have before this series in the PM
> > core. Why can't intel_runtime_pm.c be updated to use
> > pm_runtime_get_if_active() or pm_runtime_get_if_in_use() directly, and
> > make pm_runtime_get_conditional() static?
>
> Sounds like a good suggestion to me.
The i915 driver uses both but I guess it's not too much different to check
ignore_usecount separately than passing it to the API function?
I'll add another patch to do this and moving
pm_runtime_get_if_{active,in_use} implementations to runtime.c.
--
Regards,
Sakari Ailus
[PATCH v3 1/2] pm: runtime: Simplify pm_runtime_get_if_active() usage
There are two ways to opportunistically increment a device's runtime PM
usage count, calling either pm_runtime_get_if_active() or
pm_runtime_get_if_in_use(). The former has an argument to tell whether to
ignore the usage count or not, and the latter simply calls the former with
ign_usage_count set to false. The other users that want to ignore the
usage_count will have to explitly set that argument to true which is a bit
cumbersome.
To make this function more practical to use, remove the ign_usage_count
argument from the function. The main implementation is renamed as
pm_runtime_get_conditional().
Signed-off-by: Sakari Ailus
Reviewed-by: Alex Elder # drivers/net/ipa/ipa_smp2p.c
Reviewed-by: Laurent Pinchart
Acked-by: Takashi Iwai # sound/
Reviewed-by: Jacek Lawrynowicz #
drivers/accel/ivpu/
Acked-by: Rodrigo Vivi # drivers/gpu/drm/i915/
Reviewed-by: Rodrigo Vivi
---
Documentation/power/runtime_pm.rst | 5 ++--
drivers/accel/ivpu/ivpu_pm.c| 2 +-
drivers/base/power/runtime.c| 10 +---
drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
drivers/gpu/drm/xe/xe_pm.c | 2 +-
drivers/media/i2c/ccs/ccs-core.c| 2 +-
drivers/media/i2c/ov64a40.c | 2 +-
drivers/media/i2c/thp7312.c | 2 +-
drivers/net/ipa/ipa_smp2p.c | 2 +-
drivers/pci/pci.c | 2 +-
include/linux/pm_runtime.h | 32 +
sound/hda/hdac_device.c | 2 +-
12 files changed, 45 insertions(+), 20 deletions(-)
diff --git a/Documentation/power/runtime_pm.rst
b/Documentation/power/runtime_pm.rst
index 65b86e487afe..da99379071a4 100644
--- a/Documentation/power/runtime_pm.rst
+++ b/Documentation/power/runtime_pm.rst
@@ -396,10 +396,9 @@ drivers/base/power/runtime.c and
include/linux/pm_runtime.h:
nonzero, increment the counter and return 1; otherwise return 0 without
changing the counter
- `int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count);`
+ `int pm_runtime_get_if_active(struct device *dev);`
- return -EINVAL if 'power.disable_depth' is nonzero; otherwise, if the
- runtime PM status is RPM_ACTIVE, and either ign_usage_count is true
- or the device's usage_count is non-zero, increment the counter and
+ runtime PM status is RPM_ACTIVE, increment the counter and
return 1; otherwise return 0 without changing the counter
`void pm_runtime_put_noidle(struct device *dev);`
diff --git a/drivers/accel/ivpu/ivpu_pm.c b/drivers/accel/ivpu/ivpu_pm.c
index 0af8864cb3b5..c6d93c7a1c58 100644
--- a/drivers/accel/ivpu/ivpu_pm.c
+++ b/drivers/accel/ivpu/ivpu_pm.c
@@ -292,7 +292,7 @@ int ivpu_rpm_get_if_active(struct ivpu_device *vdev)
{
int ret;
- ret = pm_runtime_get_if_active(vdev->drm.dev, false);
+ ret = pm_runtime_get_if_in_use(vdev->drm.dev);
drm_WARN_ON(&vdev->drm, ret < 0);
return ret;
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 05793c9fbb84..b4cb3f19b0d8 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1176,7 +1176,7 @@ int __pm_runtime_resume(struct device *dev, int rpmflags)
EXPORT_SYMBOL_GPL(__pm_runtime_resume);
/**
- * pm_runtime_get_if_active - Conditionally bump up device usage counter.
+ * pm_runtime_get_conditional - Conditionally bump up device usage counter.
* @dev: Device to handle.
* @ign_usage_count: Whether or not to look at the current usage counter value.
*
@@ -1196,8 +1196,12 @@ EXPORT_SYMBOL_GPL(__pm_runtime_resume);
*
* The caller is responsible for decrementing the runtime PM usage counter of
* @dev after this function has returned a positive value for it.
+ *
+ * This function is not primarily intended for use in drivers, most of which
are
+ * better served by either pm_runtime_get_if_active() or
+ * pm_runtime_get_if_in_use() instead.
*/
-int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count)
+int pm_runtime_get_conditional(struct device *dev, bool ign_usage_count)
{
unsigned long flags;
int retval;
@@ -1218,7 +1222,7 @@ int pm_runtime_get_if_active(struct device *dev, bool
ign_usage_count)
return retval;
}
-EXPORT_SYMBOL_GPL(pm_runtime_get_if_active);
+EXPORT_SYMBOL_GPL(pm_runtime_get_conditional);
/**
* __pm_runtime_set_status - Set runtime PM status of a device.
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 860b51b56a92..b5f8abd2a22b 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -246,7 +246,7 @@ static intel_wakeref_t
__intel_runtime_pm_get_if_active(struct intel_runtime_pm
* function, since the power state is undefined. This applies
* atm to the late/early system suspend/resume handlers.
*/
- if
[PATCH v3 0/2] Small runtime PM API changes
Hi folks, Here's a small but a different set of patches for making two relatively minor changes to runtime PM API. I restarted version numbering as this is meaningfully different from the previous set. pm_runtime_get_if_active() loses its second argument as it only made sense to have ign_usage_count argument true. The other change is also small but it has an effect on callers: pm_runtime_put_autosuspend() will, in the future, be re-purposed to include a call to pm_runtime_mark_last_busy() as well. Before this, current users of the function are moved to __pm_runtime_put_autosuspend() (added by this patchset) which will continue to have the current behaviour. I haven't included the conversion patches in this set as I only want to do that once this set has been approved and merged. The tree specific patches can be found here, on linux-next master (there are some V4L2 patches there, too, please ignore them for now): https://git.kernel.org/pub/scm/linux/kernel/git/sailus/linux-next.git/log/?h=pm> Later on, users calling pm_runtime_mark_last_busy() immediately followed by __pm_runtime_put_autosuspend() will be switched back to pm_runtime_put_autosuspend() once its behaviour change has been done (a patch near top of that branch). I'll provide these once the preceding ones have been merged. Comments are welcome. since v2: - Rebase on v6.8-rc1 (no changes). - Add Rodrigo's Reviewed-by: to the 1st patch. since v1: - patch 1: Rename __pm_runtime_get_conditional() as pm_runtime_get_conditional(). - patch 1: Reword documentation on driver use of pm_runtime_get_conditional(). Sakari Ailus (2): pm: runtime: Simplify pm_runtime_get_if_active() usage pm: runtime: Add pm_runtime_put_autosuspend() replacement Documentation/power/runtime_pm.rst | 22 - drivers/accel/ivpu/ivpu_pm.c| 2 +- drivers/base/power/runtime.c| 10 -- drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- drivers/gpu/drm/xe/xe_pm.c | 2 +- drivers/media/i2c/ccs/ccs-core.c| 2 +- drivers/media/i2c/ov64a40.c | 2 +- drivers/media/i2c/thp7312.c | 2 +- drivers/net/ipa/ipa_smp2p.c | 2 +- drivers/pci/pci.c | 2 +- include/linux/pm_runtime.h | 44 ++--- sound/hda/hdac_device.c | 2 +- 12 files changed, 68 insertions(+), 26 deletions(-) -- 2.39.2
Re: [PATCH 3/3] media: v4l2-core: Describe privacy_led field of v4l2_subdev
Hi Bagas,
On Fri, Feb 03, 2023 at 05:02:15PM +0700, Bagas Sanjaya wrote:
> Stephen Rothwell reported htmldocs warning:
>
> include/media/v4l2-subdev.h:1088: warning: Function parameter or member
> 'privacy_led' not described in 'v4l2_subdev'
>
> Describe privacy_led field to fix the warning.
>
> Link:
> https://lore.kernel.org/linux-next/20230203135303.32da1...@canb.auug.org.au/
> Fixes: 10d96e289fbd77 ("media: v4l2-core: Make the v4l2-core code
> enable/disable the privacy LED if present")
> Reported-by: Stephen Rothwell
> Signed-off-by: Bagas Sanjaya
> ---
> include/media/v4l2-subdev.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 1ef5bbbf9d38c8..3e7a97c0657e1c 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1033,6 +1033,7 @@ struct v4l2_subdev_platform_data {
> * @active_state: Active state for the subdev (NULL for subdevs tracking the
> * state internally). Initialized by calling
> * v4l2_subdev_init_finalize().
> + * @privacy_led: Privacy LED associated with the sub-device.
> * @enabled_streams: Bitmask of enabled streams used by
> *v4l2_subdev_enable_streams() and
> *v4l2_subdev_disable_streams() helper functions for fallback
I'm not sure how this ever was an issue --- privacy_led field was
documented in the same patch that added it.
--
Kind regards,
Sakari Ailus
Re: [PATCH v13 01/10] device property: Add remote endpoint to devcon matcher
Hi Pin-yen, Thanks for the update. On Fri, Mar 03, 2023 at 10:33:41PM +0800, Pin-yen Lin wrote: > From: Prashant Malani > > When searching the device graph for device matches, check the > remote-endpoint itself for a match. > > Some drivers register devices for individual endpoints. This allows > the matcher code to evaluate those for a match too, instead > of only looking at the remote parent devices. This is required when a > device supports two mode switches in its endpoints, so we can't simply > register the mode switch with the parent node. > > Signed-off-by: Prashant Malani > Signed-off-by: Pin-yen Lin Reviewed-by: Sakari Ailus -- Sakari Ailus
Re: [PATCH v11 1/9] device property: Add remote endpoint to devcon matcher
Hi Pin-yen,
On Thu, Feb 09, 2023 at 12:28:33PM +0800, Pin-yen Lin wrote:
> Hi Sakari,
>
> Thanks for the review.
>
> On Mon, Feb 6, 2023 at 5:11 AM Sakari Ailus
> wrote:
> >
> > Hi Pin-yen,
> >
> > On Sat, Feb 04, 2023 at 09:30:32PM +0800, Pin-yen Lin wrote:
> > > From: Prashant Malani
> > >
> > > When searching the device graph for device matches, check the
> > > remote-endpoint itself for a match.
> > >
> > > Some drivers register devices for individual endpoints. This allows
> > > the matcher code to evaluate those for a match too, instead
> > > of only looking at the remote parent devices. This is required when a
> > > device supports two mode switches in its endpoints, so we can't simply
> > > register the mode switch with the parent node.
> > >
> > > Signed-off-by: Prashant Malani
> > > Signed-off-by: Pin-yen Lin
> > > Reviewed-by: Chen-Yu Tsai
> > > Tested-by: Chen-Yu Tsai
> >
> > Thanks for the update.
> >
> > I intended to give my Reviewed-by: but there's something still needs to be
> > addressed. See below.
> >
> > >
> > > ---
> > >
> > > Changes in v11:
> > > - Added missing fwnode_handle_put in drivers/base/property.c
> > >
> > > Changes in v10:
> > > - Collected Reviewed-by and Tested-by tags
> > >
> > > Changes in v6:
> > > - New in v6
> > >
> > > drivers/base/property.c | 16
> > > 1 file changed, 16 insertions(+)
> > >
> > > diff --git a/drivers/base/property.c b/drivers/base/property.c
> > > index 2a5a37fcd998..e6f915b72eb7 100644
> > > --- a/drivers/base/property.c
> > > +++ b/drivers/base/property.c
> > > @@ -1223,6 +1223,22 @@ static unsigned int
> > > fwnode_graph_devcon_matches(struct fwnode_handle *fwnode,
> > > break;
> > > }
> > >
> > > + /*
> > > + * Some drivers may register devices for endpoints. Check
> > > + * the remote-endpoints for matches in addition to the
> > > remote
> > > + * port parent.
> > > + */
> > > + node = fwnode_graph_get_remote_endpoint(ep);
> >
> > Here fwnode_graph_get_remote_endpoint() returns an endpoint...
> >
> > > + if (fwnode_device_is_available(node)) {
> >
> > and you're calling fwnode_device_is_available() on the endpoint node, which
> > always returns true.
> >
> > Shouldn't you call this on the device node instead? What about match()
> > below?
>
> Yes we should have checked the availability on the device node itself
> instead of the endpoint node. But regarding the match() call, we need
> to call it with the endpoint node because that's where we put the
> "mode-switch" properties and register the mode switches on. We can't
> use the device node because we want to register two mode switches for
> the same device node.
Ok.
I think it should be documented for both fwnode_connection_find_match() and
fwnode_connection_find_matches() may then be also called with the endpoint
node.
--
Regards,
Sakari Ailus
Re: [PATCH v11 1/9] device property: Add remote endpoint to devcon matcher
Hi Pin-yen,
On Sat, Feb 04, 2023 at 09:30:32PM +0800, Pin-yen Lin wrote:
> From: Prashant Malani
>
> When searching the device graph for device matches, check the
> remote-endpoint itself for a match.
>
> Some drivers register devices for individual endpoints. This allows
> the matcher code to evaluate those for a match too, instead
> of only looking at the remote parent devices. This is required when a
> device supports two mode switches in its endpoints, so we can't simply
> register the mode switch with the parent node.
>
> Signed-off-by: Prashant Malani
> Signed-off-by: Pin-yen Lin
> Reviewed-by: Chen-Yu Tsai
> Tested-by: Chen-Yu Tsai
Thanks for the update.
I intended to give my Reviewed-by: but there's something still needs to be
addressed. See below.
>
> ---
>
> Changes in v11:
> - Added missing fwnode_handle_put in drivers/base/property.c
>
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
>
> Changes in v6:
> - New in v6
>
> drivers/base/property.c | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 2a5a37fcd998..e6f915b72eb7 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1223,6 +1223,22 @@ static unsigned int fwnode_graph_devcon_matches(struct
> fwnode_handle *fwnode,
> break;
> }
>
> + /*
> + * Some drivers may register devices for endpoints. Check
> + * the remote-endpoints for matches in addition to the remote
> + * port parent.
> + */
> + node = fwnode_graph_get_remote_endpoint(ep);
Here fwnode_graph_get_remote_endpoint() returns an endpoint...
> + if (fwnode_device_is_available(node)) {
and you're calling fwnode_device_is_available() on the endpoint node, which
always returns true.
Shouldn't you call this on the device node instead? What about match()
below?
> + ret = match(node, con_id, data);
> + if (ret) {
> + if (matches)
> + matches[count] = ret;
> + count++;
> + }
> + }
> + fwnode_handle_put(node);
> +
> node = fwnode_graph_get_remote_port_parent(ep);
> if (!fwnode_device_is_available(node)) {
> fwnode_handle_put(node);
--
Kind regards,
Sakari Ailus
Re: [PATCH v10 1/9] device property: Add remote endpoint to devcon matcher
Hi Prashant,
On Thu, Jan 12, 2023 at 02:31:45PM -0800, Prashant Malani wrote:
> HI Sakari,
>
> On Thu, Jan 12, 2023 at 5:32 AM Sakari Ailus
> wrote:
> >
> > Hi Pin-yen,
> >
> > On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote:
> > > From: Prashant Malani
> > > + /*
> > > + * Some drivers may register devices for endpoints. Check
> > > + * the remote-endpoints for matches in addition to the
> > > remote
> > > + * port parent.
> > > + */
> > > + node = fwnode_graph_get_remote_endpoint(ep);
> > > + if (fwnode_device_is_available(node)) {
> > > + ret = match(node, con_id, data);
> > > + if (ret) {
> > > + if (matches)
> > > + matches[count] = ret;
> > > + count++;
> > > + }
> > > + }
> >
> > Aren't you missing fwnode_handle-put(node) here??
>
> It shouldn't be necessary. We aren't break-ing/continue-ing here,
> and fwnode_handle_put(node) is called latter in the loop [1][2]
It is, but node is overwritten just below this chunk --- before
fwnode_handle_put() is called on it.
>
> BR,
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/property.c#n1256
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/property.c#n1261
--
Regards,
Sakari Ailus
Re: [PATCH v10 1/9] device property: Add remote endpoint to devcon matcher
Hi Pin-yen,
On Thu, Jan 12, 2023 at 12:20:56PM +0800, Pin-yen Lin wrote:
> From: Prashant Malani
>
> When searching the device graph for device matches, check the
> remote-endpoint itself for a match.
>
> Some drivers register devices for individual endpoints. This allows
> the matcher code to evaluate those for a match too, instead
> of only looking at the remote parent devices. This is required when a
> device supports two mode switches in its endpoints, so we can't simply
> register the mode switch with the parent node.
>
> Signed-off-by: Prashant Malani
> Signed-off-by: Pin-yen Lin
> Reviewed-by: Chen-Yu Tsai
> Tested-by: Chen-Yu Tsai
>
> ---
>
> Changes in v10:
> - Collected Reviewed-by and Tested-by tags
>
> Changes in v6:
> - New in v6
>
> drivers/base/property.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 2a5a37fcd998..48877af4e444 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1223,6 +1223,21 @@ static unsigned int fwnode_graph_devcon_matches(struct
> fwnode_handle *fwnode,
> break;
> }
>
> + /*
> + * Some drivers may register devices for endpoints. Check
> + * the remote-endpoints for matches in addition to the remote
> + * port parent.
> + */
> + node = fwnode_graph_get_remote_endpoint(ep);
> + if (fwnode_device_is_available(node)) {
> + ret = match(node, con_id, data);
> + if (ret) {
> + if (matches)
> + matches[count] = ret;
> + count++;
> + }
> + }
Aren't you missing fwnode_handle-put(node) here??
> +
> node = fwnode_graph_get_remote_port_parent(ep);
> if (!fwnode_device_is_available(node)) {
> fwnode_handle_put(node);
--
Kind regards,
Sakari Ailus
Re: [PATCH v2 7/7] drm: rcar-du: Add new formats (2-10-10-10 ARGB, Y210)
Hi Laurent,
On Mon, Dec 19, 2022 at 11:47:04PM +0200, Laurent Pinchart wrote:
> Hi Tomi,
>
> (CC'ing Sakari and Hans)
>
> Thank you for the patch.
>
> On Mon, Dec 19, 2022 at 04:01:39PM +0200, Tomi Valkeinen wrote:
> > Add new pixel formats: RGBX1010102, RGBA1010102, ARGB2101010 and Y210.
> >
> > Signed-off-by: Tomi Valkeinen
> > ---
> > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 24 +
> > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 49 +--
> > 2 files changed, 71 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > index 8c2719efda2a..8ccabf5a30c4 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -259,6 +259,24 @@ static const struct rcar_du_format_info
> > rcar_du_format_infos[] = {
> > .bpp = 32,
> > .planes = 1,
> > .hsub = 1,
> > + }, {
> > + .fourcc = DRM_FORMAT_RGBX1010102,
>
> Ah, here the format makes sense.
>
> > + .v4l2 = V4L2_PIX_FMT_XBGR2101010,
>
> But this is horrible :-( Could we use the same names as DRM for new
> formats, when there is no conflict with existing V4L2 formats ?
>
> Sakari, Hans, what do you think ? Please see patch 1/7 in the series for
> the format definitions.
I think it'd be good to have only one set of definitions.
Can we can sort the endianness question in a reasonable way?
Also new Bayer formats will probably be still needed on V4L2 side but will
they be relevant for DRM? I suppose that would mean new DRM format for
each pixel order, too? Or can we think of something smarter that would
still work reasonably with existing formats?
--
Kind regards,
Sakari Ailus
Re: [PATCH v3 2/6] dt-bindings: Use new video interface bus type macros in examples
Hi Laurent,
On Sat, Jul 16, 2022 at 06:12:05PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Sat, Jul 16, 2022 at 12:28:45PM +0300, Sakari Ailus wrote:
> > On Thu, Jun 16, 2022 at 01:14:06AM +0300, Laurent Pinchart wrote:
> > > Now that a header exists with macros for the media interface bus-type
> > > values, replace hardcoding numerical constants with the corresponding
> > > macros in the DT binding examples.
> > >
> > > Signed-off-by: Laurent Pinchart
> > > ---
> > > Changes since v2:
> > >
> > > - Go back to PARALLEL
> > >
> > > Changes since v1:
> > >
> > > - Rename PARALLEL to BT601
> > > ---
> > > .../devicetree/bindings/display/bridge/analogix,anx7625.yaml | 1 +
> > > Documentation/devicetree/bindings/media/i2c/mipi-ccs.yaml | 3 ++-
> > > Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml | 3 ++-
> > > .../devicetree/bindings/media/marvell,mmp2-ccic.yaml | 3 ++-
> > > Documentation/devicetree/bindings/media/microchip,xisc.yaml | 3 ++-
> > > Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml| 4 +++-
> > > 6 files changed, 12 insertions(+), 5 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > > b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > > index 35a48515836e..b0e5585f93e2 100644
> > > ---
> > > a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > > +++
> > > b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > > @@ -118,6 +118,7 @@ additionalProperties: false
> > > examples:
> > >- |
> > > #include
> > > +#include
> > >
> > > i2c0 {
> > > #address-cells = <1>;
> >
> > The definition doesn't seem to be used here. Is there a need to include
> > this?
>
> There was, but the change that added bus-type to this binding got
> reverted in commit 979452fbc430 ("dt-bindings: drm/bridge: anx7625:
> Revert DPI support") and I forgot to drop the header when rebasing.
>
> > I could drop this chunk while applying. There's just one trivial change
> > elsewhere in this patch to make.
>
> Please do :-)
Applied with the following diff:
diff --git
a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
index 7fa7a667c764a..4590186c4a0b8 100644
--- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
@@ -133,7 +133,6 @@ additionalProperties: false
examples:
- |
#include
-#include
i2c0 {
#address-cells = <1>;
diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
index 285c6075950ad..e80fcdf280f0b 100644
--- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
+++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
@@ -92,7 +92,7 @@ examples:
#include
#include
#include
-#
+
dcmi: dcmi@4c006000 {
compatible = "st,stm32-dcmi";
reg = <0x4c006000 0x400>;
It seems that there's also bus-type 7 that corresponds to MIPI DPI. Could
you add a macro for this one as well? :-) In particular it's used by
Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml .
--
Kind regards,
Sakari Ailus
Re: [PATCH v3 0/6] dt-bindings: Add macros for video interface bus types
Hi Laurent, On Sat, Nov 19, 2022 at 09:15:04PM +0200, Laurent Pinchart wrote: > Hello, > > On Fri, Nov 18, 2022 at 06:23:38PM +0900, Paul Elder wrote: > > Hi Sakari, > > > > Gentle ping. > > > > On Sun, Jul 17, 2022 at 06:54:00AM +, Sakari Ailus wrote: > > > Folks, > > > > > > > Laurent Pinchart (6): > > > > dt-bindings: media: Add macros for video interface bus types > > > > dt-bindings: Use new video interface bus type macros in examples > > > > ARM: dts: freescale: Use new media bus type macros > > > > ARM: dts: omap: Use new media bus type macros > > > > ARM: dts: renesas: Use new media bus type macros > > > > ARM: dts: stm32: Use new media bus type macros > > > > > > What's the preference on the tree through which these would be merged? > > > > > > The two first should probably go through the media tree but what about the > > > DTS? There's a dependency to the first patch. I can take these all if > > > people are fine with that. > > > > How is this going? > > Sakari, if there's a concern taking patches 3/6 to 6/6 in the media > tree, could you merge 1/6 and 2/6 ? If they can still be included in a > pull request for v6.2, I'll work on getting the dts changes in v6.3 > through their respective trees. I think I was expecting v4 from you. But I can address the comments, too, they were minor matters. For applying the DTS patches I'd like to have ack from respective treemaintainers it's fine to take these through the media tree. -- Kind regards, Sakari Ailus
Re: [PATCH v3 0/6] dt-bindings: Add macros for video interface bus types
Folks, > Laurent Pinchart (6): > dt-bindings: media: Add macros for video interface bus types > dt-bindings: Use new video interface bus type macros in examples > ARM: dts: freescale: Use new media bus type macros > ARM: dts: omap: Use new media bus type macros > ARM: dts: renesas: Use new media bus type macros > ARM: dts: stm32: Use new media bus type macros What's the preference on the tree through which these would be merged? The two first should probably go through the media tree but what about the DTS? There's a dependency to the first patch. I can take these all if people are fine with that. -- Kind regards, Sakari Ailus
Re: [PATCH v3 2/6] dt-bindings: Use new video interface bus type macros in examples
Hi Laurent,
On Thu, Jun 16, 2022 at 01:14:06AM +0300, Laurent Pinchart wrote:
> Now that a header exists with macros for the media interface bus-type
> values, replace hardcoding numerical constants with the corresponding
> macros in the DT binding examples.
>
> Signed-off-by: Laurent Pinchart
> ---
> Changes since v2:
>
> - Go back to PARALLEL
>
> Changes since v1:
>
> - Rename PARALLEL to BT601
> ---
> .../devicetree/bindings/display/bridge/analogix,anx7625.yaml | 1 +
> Documentation/devicetree/bindings/media/i2c/mipi-ccs.yaml | 3 ++-
> Documentation/devicetree/bindings/media/i2c/ovti,ov772x.yaml | 3 ++-
> .../devicetree/bindings/media/marvell,mmp2-ccic.yaml | 3 ++-
> Documentation/devicetree/bindings/media/microchip,xisc.yaml | 3 ++-
> Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml| 4 +++-
> 6 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> index 35a48515836e..b0e5585f93e2 100644
> --- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> @@ -118,6 +118,7 @@ additionalProperties: false
> examples:
>- |
> #include
> +#include
>
> i2c0 {
> #address-cells = <1>;
The definition doesn't seem to be used here. Is there a need to include
this?
I could drop this chunk while applying. There's just one trivial change
elsewhere in this patch to make.
--
Kind regards,
Sakari Ailus
Re: [PATCH v9 00/14] Add some DRM bridge drivers support for i.MX8qm/qxp SoCs
On Sat, Jun 11, 2022 at 10:14:07PM +0800, Liu Ying wrote: > Patch 1/14 and 2/14 add bus formats used by pixel combiner. Thanks! For these: Acked-by: Sakari Ailus -- Sakari Ailus
Re: [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
Hi Lucas, On Tue, Jan 18, 2022 at 11:24:48PM -0800, Lucas De Marchi wrote: > @@ -1354,8 +1345,7 @@ static bool tomoyo_print_condition(struct > tomoyo_io_buffer *head, > case 3: > if (cond->grant_log != TOMOYO_GRANTLOG_AUTO) > tomoyo_io_printf(head, " grant_log=%s", > - tomoyo_yesno(cond->grant_log == > - TOMOYO_GRANTLOG_YES)); > + yesno(cond->grant_log == > TOMOYO_GRANTLOG_YES)); This would be better split on two lines. Then, Reviewed-by: Sakari Ailus -- Sakari Ailus
Re: [PATCH] media: switch from 'pci_' to 'dma_' API
Thanks, Christophe. On Sun, Aug 22, 2021 at 11:30:08AM +0200, Christophe JAILLET wrote: > The wrappers in include/linux/pci-dma-compat.h should go away. > > The patch has been generated with the coccinelle script below. > > It has been compile tested. Reviewed-by: Sakari Ailus -- Sakari Ailus
Re: [PATCH 0/4] drm: Finally retire struct drm_format_name_buf
On Wed, May 26, 2021 at 09:21:10PM +0200, Thomas Zimmermann wrote: > ping for further a-bs / r-bs Thanks for the ping. For the series: Reviewed-by: Sakari Ailus > > Am 16.05.21 um 14:13 schrieb Thomas Zimmermann: > > This is a cleanup patchset to remove drm_format_name_buf et al. There > > are two instances in drivers that need to be replaced with the %4cc > > printk format modifier. Patch 3 was left over back from an earlier > > patchset. [1] Patch 4 removes struct drm_format_name_buf. > > > > I built-tested with drm-tip. The patchsetcan be mered through drm-misc. > > > > [1] > > https://lore.kernel.org/dri-devel/20210216155723.17109-1-sakari.ai...@linux.intel.com/ > > > > Sakari Ailus (1): > >drm: Remove drm_get_format_name() > > > > Thomas Zimmermann (3): > >drm/amdgpu: Use %p4cc to print 4CC format > >drm/simpledrm: Use %p4cc to print 4CC format > >drm/fourcc: Remove struct drm_format_buf_name > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 7 ++ > > drivers/gpu/drm/drm_fourcc.c| 25 - > > drivers/gpu/drm/tiny/simpledrm.c| 6 ++--- > > include/drm/drm_fourcc.h| 9 > > 4 files changed, 4 insertions(+), 43 deletions(-) > > > > -- > > 2.31.1 > > > -- Sakari Ailus
Re: [PATCH 5/9] drm/i915: Associate ACPI connector nodes with connector entries
Hi Andy, Hans, On Wed, May 05, 2021 at 12:17:14PM +0300, Andy Shevchenko wrote: > On Wed, May 5, 2021 at 12:07 PM Hans de Goede wrote: > > On 5/4/21 9:52 AM, Andy Shevchenko wrote: > > > On Monday, May 3, 2021, Hans de Goede > > <mailto:hdego...@redhat.com>> wrote: > > ... > > > > + fwnode = device_get_next_child_node(kdev, fwnode); > > > > Who is dropping reference counting on fwnode ? > > > > We are dealing with ACPI fwnode-s here and those are not ref-counted, they > > are embedded inside a struct acpi_device and their lifetime is tied to > > that struct. They should probably still be ref-counted (with the count > > never dropping to 0) so that the generic fwnode functions behave the same > > anywhere but atm the ACPI nodes are not refcounted, see: > > acpi_get_next_subnode() > > in drivers/acpi/property.c which is the get_next_child_node() implementation > > for ACPI fwnode-s. > > Yes, ACPI currently is exceptional, but fwnode API is not. > If you may guarantee that this case won't ever be outside of ACPI and > even though if ACPI won't ever gain a reference counting for fwnodes, > we can leave it as is. > > > > I’m in the middle of a pile of fixes for fwnode refcounting when > > > for_each_child or get_next_child is used. So, please double check you > > > drop a reference. > > > > The kdoc comments on device_get_next_child_node() / > > fwnode_get_next_child_node() > > do not mention anything about these functions returning a reference. > > It's possible. I dunno if it had to be done earlier. Sakari? The fwnode API has worked with references for a long time, looks like from the very beginning of it, as in patch 8a0662d9ed2968e1186208336a8e1fab3fdfea63 . If you're expecting an fwnode family of function returning another node, then that function has to have taken a reference to that node before returning it. Otherwise there's no guarantee that node is still there when it is returned. It may be this is not documented for every function doing so. That should probably be added. -- Kind regards, Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v9 0/4] Add %p4cc printk modifier for V4L2 and DRM fourcc codes
On Wed, Feb 17, 2021 at 01:14:42PM +0100, Thomas Zimmermann wrote: > Hi > > Am 16.02.21 um 16:57 schrieb Sakari Ailus: > > Hi all, > > > > On merging --- it would seem everyone is happy with merging this > > through the drm-misc tree. The last patch should wait until all > > users are gone for sure, probably to the next kernel release. > > There are no users of drm_get_format_name() in linux-next > > currently after the 3rd patch. > > I've merged patches 1 to 3 into drm-misc-next. Patch 4 (and maybe some final > fix-up patch) will land when all DRM trees have catched up the changes. Thank you! -- Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v9 3/4] drm: Switch to %p4cc format modifier
Switch DRM drivers from drm_get_format_name() to %p4cc. This gets rid of a
large number of temporary variables at the same time.
Signed-off-by: Sakari Ailus
Reviewed-by: Petr Mladek
Reviewed-by: Andy Shevchenko
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 5 ++--
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 5 ++--
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 5 ++--
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 5 ++--
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++--
.../arm/display/komeda/komeda_format_caps.h | 11 ---
.../arm/display/komeda/komeda_framebuffer.c | 4 +--
.../gpu/drm/arm/display/komeda/komeda_plane.c | 6 ++--
drivers/gpu/drm/arm/malidp_mw.c | 7 ++---
drivers/gpu/drm/drm_atomic.c | 8 ++---
drivers/gpu/drm/drm_crtc.c| 7 ++---
drivers/gpu/drm/drm_framebuffer.c | 11 ++-
drivers/gpu/drm/drm_mipi_dbi.c| 5 ++--
drivers/gpu/drm/drm_plane.c | 8 ++---
.../gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 5 ++--
drivers/gpu/drm/i915/display/intel_display.c | 14 +++--
.../drm/i915/display/intel_display_debugfs.c | 30 ---
drivers/gpu/drm/i915/display/intel_sprite.c | 6 ++--
drivers/gpu/drm/mcde/mcde_display.c | 6 ++--
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 ++--
drivers/gpu/drm/nouveau/nouveau_display.c | 9 ++
drivers/gpu/drm/radeon/atombios_crtc.c| 10 +++
drivers/gpu/drm/sun4i/sun4i_backend.c | 6 ++--
drivers/gpu/drm/vkms/vkms_writeback.c | 7 ++---
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 --
25 files changed, 69 insertions(+), 137 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index 7944781e1086..ea825b4f8ee8 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -1862,7 +1862,6 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc
*crtc,
u32 tmp, viewport_w, viewport_h;
int r;
bool bypass_lut = false;
- struct drm_format_name_buf format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -1981,8 +1980,8 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc
*crtc,
#endif
break;
default:
- DRM_ERROR("Unsupported screen format %s\n",
- drm_get_format_name(target_fb->format->format,
&format_name));
+ DRM_ERROR("Unsupported screen format %p4cc\n",
+ &target_fb->format->format);
return -EINVAL;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index 1b6ff0470011..a360a6dec198 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -1904,7 +1904,6 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc
*crtc,
u32 tmp, viewport_w, viewport_h;
int r;
bool bypass_lut = false;
- struct drm_format_name_buf format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -2023,8 +2022,8 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc
*crtc,
#endif
break;
default:
- DRM_ERROR("Unsupported screen format %s\n",
- drm_get_format_name(target_fb->format->format,
&format_name));
+ DRM_ERROR("Unsupported screen format %p4cc\n",
+ &target_fb->format->format);
return -EINVAL;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index 83a88385b762..ef124ac853b6 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
@@ -1820,7 +1820,6 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc
*crtc,
u32 viewport_w, viewport_h;
int r;
bool bypass_lut = false;
- struct drm_format_name_buf format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -1929,8 +1928,8 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc
*crtc,
#endif
break;
default:
- DRM_ERROR("Unsupported screen format %s\n",
- drm_get_format_name(target_fb->format->format,
&format_name));
+ DRM_ERROR("Unsupported screen format %p4cc\n",
+ &target_fb->format->format);
return -EINVAL;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index 224b30214427..c98650183828 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/
[PATCH v9 2/4] v4l: ioctl: Use %p4cc printk modifier to print FourCC codes
Now that we can print FourCC codes directly using printk, make use of the
feature in V4L2 core.
Signed-off-by: Sakari Ailus
Acked-by: Mauro Carvalho Chehab
Reviewed-by: Petr Mladek
Reviewed-by: Andy Shevchenko
---
drivers/media/v4l2-core/v4l2-ioctl.c | 85 +++-
1 file changed, 21 insertions(+), 64 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
b/drivers/media/v4l2-core/v4l2-ioctl.c
index 31d1342e61e8..31662c3a8c9e 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -265,13 +265,9 @@ static void v4l_print_fmtdesc(const void *arg, bool
write_only)
{
const struct v4l2_fmtdesc *p = arg;
- pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%c%c%c%c,
mbus_code=0x%04x, description='%.*s'\n",
+ pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%p4cc,
mbus_code=0x%04x, description='%.*s'\n",
p->index, prt_names(p->type, v4l2_type_names),
- p->flags, (p->pixelformat & 0xff),
- (p->pixelformat >> 8) & 0xff,
- (p->pixelformat >> 16) & 0xff,
- (p->pixelformat >> 24) & 0xff,
- p->mbus_code,
+ p->flags, &p->pixelformat, p->mbus_code,
(int)sizeof(p->description), p->description);
}
@@ -293,12 +289,8 @@ static void v4l_print_format(const void *arg, bool
write_only)
case V4L2_BUF_TYPE_VIDEO_CAPTURE:
case V4L2_BUF_TYPE_VIDEO_OUTPUT:
pix = &p->fmt.pix;
- pr_cont(", width=%u, height=%u, pixelformat=%c%c%c%c, field=%s,
bytesperline=%u, sizeimage=%u, colorspace=%d, flags=0x%x, ycbcr_enc=%u,
quantization=%u, xfer_func=%u\n",
- pix->width, pix->height,
- (pix->pixelformat & 0xff),
- (pix->pixelformat >> 8) & 0xff,
- (pix->pixelformat >> 16) & 0xff,
- (pix->pixelformat >> 24) & 0xff,
+ pr_cont(", width=%u, height=%u, pixelformat=%p4cc, field=%s,
bytesperline=%u, sizeimage=%u, colorspace=%d, flags=0x%x, ycbcr_enc=%u,
quantization=%u, xfer_func=%u\n",
+ pix->width, pix->height, &pix->pixelformat,
prt_names(pix->field, v4l2_field_names),
pix->bytesperline, pix->sizeimage,
pix->colorspace, pix->flags, pix->ycbcr_enc,
@@ -307,12 +299,8 @@ static void v4l_print_format(const void *arg, bool
write_only)
case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
mp = &p->fmt.pix_mp;
- pr_cont(", width=%u, height=%u, format=%c%c%c%c, field=%s,
colorspace=%d, num_planes=%u, flags=0x%x, ycbcr_enc=%u, quantization=%u,
xfer_func=%u\n",
- mp->width, mp->height,
- (mp->pixelformat & 0xff),
- (mp->pixelformat >> 8) & 0xff,
- (mp->pixelformat >> 16) & 0xff,
- (mp->pixelformat >> 24) & 0xff,
+ pr_cont(", width=%u, height=%u, format=%p4cc, field=%s,
colorspace=%d, num_planes=%u, flags=0x%x, ycbcr_enc=%u, quantization=%u,
xfer_func=%u\n",
+ mp->width, mp->height, &mp->pixelformat,
prt_names(mp->field, v4l2_field_names),
mp->colorspace, mp->num_planes, mp->flags,
mp->ycbcr_enc, mp->quantization, mp->xfer_func);
@@ -337,13 +325,9 @@ static void v4l_print_format(const void *arg, bool
write_only)
case V4L2_BUF_TYPE_VBI_CAPTURE:
case V4L2_BUF_TYPE_VBI_OUTPUT:
vbi = &p->fmt.vbi;
- pr_cont(", sampling_rate=%u, offset=%u, samples_per_line=%u,
sample_format=%c%c%c%c, start=%u,%u, count=%u,%u\n",
+ pr_cont(", sampling_rate=%u, offset=%u, samples_per_line=%u,
sample_format=%p4cc, start=%u,%u, count=%u,%u\n",
vbi->sampling_rate, vbi->offset,
- vbi->samples_per_line,
- (vbi->sample_format & 0xff),
- (vbi->sample_format >> 8) & 0xff,
- (vbi->sample_format >> 16) & 0xff,
- (vbi->sample_format >> 24) & 0xff,
+ vbi->samples_per_line, &vbi->sample_format,
vbi->start[0], vbi->start[1],
vbi->count[0], vbi->count[1]);
break;
@@ -360,21 +344,13 @@ static void
[PATCH v9 0/4] Add %p4cc printk modifier for V4L2 and DRM fourcc codes
Hi all,
On merging --- it would seem everyone is happy with merging this
through the drm-misc tree. The last patch should wait until all
users are gone for sure, probably to the next kernel release.
There are no users of drm_get_format_name() in linux-next
currently after the 3rd patch.
This set adds support for %p4cc printk modifier for printing V4L2 and DRM
fourcc codes. The codes are cumbersome to print manually and by adding the
modifier, this task is saved from the V4L2 and DRM frameworks as well as
related drivers. DRM actually had it handled in a way (see 3rd patch) but
the printk modifier makes printing the format easier even there. On V4L2
side it saves quite a few lines of repeating different implementations of
printing the 4cc codes.
Further work will include converting the V4L2 drivers doing the same. I
left these out from this version since individual drivers are easier
changed without dealing with multiple trees.
Since v8:
- Reduce ternary conditionals in intel_plane_uapi_info().
- Wrap a long line in intel_plane_hw_info().
Since v7:
- Add more examples, one with big endian and another with a space.
- Add Y10 test format.
- Use "0123" in the size string for temporary buffer.
- Added acks.
- Split the 3rd patch into two: driver changes and removal of
drm_get_format_name().
Since v6:
- Don't drop spaces in fourcc codes.
- Print unprintable characters as dot ('.') instead of hexadecimal number
in parentheses.
- Convert DRM from drm_get_format_name() to %p4cc. I wonder if this should
be merged through the DRM tree, albeit it's probably unlikely to
conflict with other changes. Further use of the function could be a
problem.
- Make tests more realistic.
Since v5:
- Added V4L2 core conversion to %p4cc, as well as change the DRM
fourcc printing function to use %p4cc.
- Add missing checkpatch.pl checks for %p4cc modifier.
Sakari Ailus (4):
lib/vsprintf: Add support for printing V4L2 and DRM fourccs
v4l: ioctl: Use %p4cc printk modifier to print FourCC codes
drm: Switch to %p4cc format modifier
drm: Remove drm_get_format_name()
Documentation/core-api/printk-formats.rst | 18
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 5 +-
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 5 +-
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 5 +-
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 5 +-
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +-
.../arm/display/komeda/komeda_format_caps.h | 11 ---
.../arm/display/komeda/komeda_framebuffer.c | 4 +-
.../gpu/drm/arm/display/komeda/komeda_plane.c | 6 +-
drivers/gpu/drm/arm/malidp_mw.c | 7 +-
drivers/gpu/drm/drm_atomic.c | 8 +-
drivers/gpu/drm/drm_crtc.c| 7 +-
drivers/gpu/drm/drm_fourcc.c | 25 --
drivers/gpu/drm/drm_framebuffer.c | 11 +--
drivers/gpu/drm/drm_mipi_dbi.c| 5 +-
drivers/gpu/drm/drm_plane.c | 8 +-
.../gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 5 +-
drivers/gpu/drm/i915/display/intel_display.c | 14 +--
.../drm/i915/display/intel_display_debugfs.c | 19 ++---
drivers/gpu/drm/i915/display/intel_sprite.c | 6 +-
drivers/gpu/drm/mcde/mcde_display.c | 6 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 +-
drivers/gpu/drm/nouveau/nouveau_display.c | 9 +-
drivers/gpu/drm/radeon/atombios_crtc.c| 10 +--
drivers/gpu/drm/sun4i/sun4i_backend.c | 6 +-
drivers/gpu/drm/vkms/vkms_writeback.c | 7 +-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 ++--
drivers/media/v4l2-core/v4l2-ioctl.c | 85 +--
include/drm/drm_fourcc.h | 1 -
lib/test_printf.c | 18
lib/vsprintf.c| 39 +
scripts/checkpatch.pl | 6 +-
32 files changed, 164 insertions(+), 223 deletions(-)
--
2.29.2
*** BLURB HERE ***
Sakari Ailus (4):
lib/vsprintf: Add support for printing V4L2 and DRM fourccs
v4l: ioctl: Use %p4cc printk modifier to print FourCC codes
drm: Switch to %p4cc format modifier
drm: Remove drm_get_format_name()
Documentation/core-api/printk-formats.rst | 18
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 5 +-
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 5 +-
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 5 +-
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 5 +-
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +-
.../arm/display/komeda/komeda_format_caps.h | 11 ---
.../arm/display/komeda/komeda_framebuffer.c | 4 +-
.../gpu/drm/arm/display/komeda/komeda_plane.c | 6 +-
drivers/gpu/drm/arm/malidp_mw.c | 7 +-
drivers/gpu/drm/drm_atomic.c | 8 +-
drivers/gpu/drm/drm_crtc.c| 7 +-
drivers/gpu/drm/drm_fourcc.c
[PATCH v9 1/4] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
pixel formats denoted by fourccs. The fourcc encoding is the same for both
so the same implementation can be used.
Suggested-by: Mauro Carvalho Chehab
Signed-off-by: Sakari Ailus
Reviewed-by: Petr Mladek
Reviewed-by: Sergey Senozhatsky
Reviewed-by: Andy Shevchenko
---
Documentation/core-api/printk-formats.rst | 18 +++
lib/test_printf.c | 18 +++
lib/vsprintf.c| 39 +++
scripts/checkpatch.pl | 6 ++--
4 files changed, 79 insertions(+), 2 deletions(-)
diff --git a/Documentation/core-api/printk-formats.rst
b/Documentation/core-api/printk-formats.rst
index 160e710d992f..8267675ea95c 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -567,6 +567,24 @@ For printing netdev_features_t.
Passed by reference.
+V4L2 and DRM FourCC code (pixel format)
+---
+
+::
+
+ %p4cc
+
+Print a FourCC code used by V4L2 or DRM, including format endianness and
+its numerical value as hexadecimal.
+
+Passed by reference.
+
+Examples::
+
+ %p4cc BG12 little-endian (0x32314742)
+ %p4cc Y10 little-endian (0x20303159)
+ %p4cc NV12 big-endian (0xb231564e)
+
Thanks
==
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7d60f24240a4..8558ddbde692 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -647,6 +647,23 @@ static void __init fwnode_pointer(void)
software_node_unregister_nodes(softnodes);
}
+static void __init fourcc_pointer(void)
+{
+ struct {
+ u32 code;
+ char *str;
+ } const try[] = {
+ { 0x3231564e, "NV12 little-endian (0x3231564e)", },
+ { 0xb231564e, "NV12 big-endian (0xb231564e)", },
+ { 0x10111213, " little-endian (0x10111213)", },
+ { 0x20303159, "Y10 little-endian (0x20303159)", },
+ };
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(try); i++)
+ test(try[i].str, "%p4cc", &try[i].code);
+}
+
static void __init
errptr(void)
{
@@ -692,6 +709,7 @@ test_pointer(void)
flags();
errptr();
fwnode_pointer();
+ fourcc_pointer();
}
static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..62c7224a5076 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1733,6 +1733,42 @@ char *netdev_bits(char *buf, char *end, const void *addr,
return special_hex_number(buf, end, num, size);
}
+static noinline_for_stack
+char *fourcc_string(char *buf, char *end, const u32 *fourcc,
+ struct printf_spec spec, const char *fmt)
+{
+ char output[sizeof("0123 little-endian (0x01234567)")];
+ char *p = output;
+ unsigned int i;
+ u32 val;
+
+ if (fmt[1] != 'c' || fmt[2] != 'c')
+ return error_string(buf, end, "(%p4?)", spec);
+
+ if (check_pointer(&buf, end, fourcc, spec))
+ return buf;
+
+ val = *fourcc & ~BIT(31);
+
+ for (i = 0; i < sizeof(*fourcc); i++) {
+ unsigned char c = val >> (i * 8);
+
+ /* Print non-control ASCII characters as-is, dot otherwise */
+ *p++ = isascii(c) && isprint(c) ? c : '.';
+ }
+
+ strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
+ p += strlen(p);
+
+ *p++ = ' ';
+ *p++ = '(';
+ p = special_hex_number(p, output + sizeof(output) - 2, *fourcc,
sizeof(u32));
+ *p++ = ')';
+ *p = '\0';
+
+ return string(buf, end, output, spec);
+}
+
static noinline_for_stack
char *address_val(char *buf, char *end, const void *addr,
struct printf_spec spec, const char *fmt)
@@ -2162,6 +2198,7 @@ char *fwnode_string(char *buf, char *end, struct
fwnode_handle *fwnode,
* correctness of the format string and va_list arguments.
* - 'K' For a kernel pointer that should be hidden from unprivileged users
* - 'NF' For a netdev_features_t
+ * - '4cc' V4L2 or DRM FourCC code, with endianness and raw numerical value.
* - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
*a certain separator (' ' by default):
* C colon
@@ -2259,6 +2296,8 @@ char *pointer(const char *fmt, char *buf, char *end, void
*ptr,
return restricted_pointer(buf, end, ptr, spec);
case 'N':
return netdev_bits(buf, end, ptr, spec, fmt);
+ case '4':
+ return fourcc_string(buf, end, ptr, spec, fmt);
case 'a&
[PATCH v9 4/4] drm: Remove drm_get_format_name()
The %p4cc printk format modifier was recently added to print fourcc codes,
replacing drm_get_format_name(). The function is no longer needed, so
remove it.
Signed-off-by: Sakari Ailus
Reviewed-by: Petr Mladek
Reviewed-by: Andy Shevchenko
---
drivers/gpu/drm/drm_fourcc.c | 25 -
include/drm/drm_fourcc.h | 1 -
2 files changed, 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 03262472059c..5cf45aa6eedc 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -30,11 +30,6 @@
#include
#include
-static char printable_char(int c)
-{
- return isascii(c) && isprint(c) ? c : '?';
-}
-
/**
* drm_mode_legacy_fb_format - compute drm fourcc code from legacy description
* @bpp: bits per pixels
@@ -130,26 +125,6 @@ uint32_t drm_driver_legacy_fb_format(struct drm_device
*dev,
}
EXPORT_SYMBOL(drm_driver_legacy_fb_format);
-/**
- * drm_get_format_name - fill a string with a drm fourcc format's name
- * @format: format to compute name of
- * @buf: caller-supplied buffer
- */
-const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf
*buf)
-{
- snprintf(buf->str, sizeof(buf->str),
-"%c%c%c%c %s-endian (0x%08x)",
-printable_char(format & 0xff),
-printable_char((format >> 8) & 0xff),
-printable_char((format >> 16) & 0xff),
-printable_char((format >> 24) & 0x7f),
-format & DRM_FORMAT_BIG_ENDIAN ? "big" : "little",
-format);
-
- return buf->str;
-}
-EXPORT_SYMBOL(drm_get_format_name);
-
/*
* Internal function to query information for a given format. See
* drm_format_info() for the public API.
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 156b122c0ad5..3ea17b8a79d3 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -318,6 +318,5 @@ unsigned int drm_format_info_block_height(const struct
drm_format_info *info,
int plane);
uint64_t drm_format_info_min_pitch(const struct drm_format_info *info,
int plane, unsigned int buffer_width);
-const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf
*buf);
#endif /* __DRM_FOURCC_H__ */
--
2.29.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 1/4] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
pixel formats denoted by fourccs. The fourcc encoding is the same for both
so the same implementation can be used.
Suggested-by: Mauro Carvalho Chehab
Signed-off-by: Sakari Ailus
Reviewed-by: Petr Mladek
Reviewed-by: Sergey Senozhatsky
Reviewed-by: Andy Shevchenko
---
Documentation/core-api/printk-formats.rst | 18 +++
lib/test_printf.c | 18 +++
lib/vsprintf.c| 39 +++
scripts/checkpatch.pl | 6 ++--
4 files changed, 79 insertions(+), 2 deletions(-)
diff --git a/Documentation/core-api/printk-formats.rst
b/Documentation/core-api/printk-formats.rst
index 160e710d992f..8267675ea95c 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -567,6 +567,24 @@ For printing netdev_features_t.
Passed by reference.
+V4L2 and DRM FourCC code (pixel format)
+---
+
+::
+
+ %p4cc
+
+Print a FourCC code used by V4L2 or DRM, including format endianness and
+its numerical value as hexadecimal.
+
+Passed by reference.
+
+Examples::
+
+ %p4cc BG12 little-endian (0x32314742)
+ %p4cc Y10 little-endian (0x20303159)
+ %p4cc NV12 big-endian (0xb231564e)
+
Thanks
==
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7d60f24240a4..8558ddbde692 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -647,6 +647,23 @@ static void __init fwnode_pointer(void)
software_node_unregister_nodes(softnodes);
}
+static void __init fourcc_pointer(void)
+{
+ struct {
+ u32 code;
+ char *str;
+ } const try[] = {
+ { 0x3231564e, "NV12 little-endian (0x3231564e)", },
+ { 0xb231564e, "NV12 big-endian (0xb231564e)", },
+ { 0x10111213, " little-endian (0x10111213)", },
+ { 0x20303159, "Y10 little-endian (0x20303159)", },
+ };
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(try); i++)
+ test(try[i].str, "%p4cc", &try[i].code);
+}
+
static void __init
errptr(void)
{
@@ -692,6 +709,7 @@ test_pointer(void)
flags();
errptr();
fwnode_pointer();
+ fourcc_pointer();
}
static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..62c7224a5076 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1733,6 +1733,42 @@ char *netdev_bits(char *buf, char *end, const void *addr,
return special_hex_number(buf, end, num, size);
}
+static noinline_for_stack
+char *fourcc_string(char *buf, char *end, const u32 *fourcc,
+ struct printf_spec spec, const char *fmt)
+{
+ char output[sizeof("0123 little-endian (0x01234567)")];
+ char *p = output;
+ unsigned int i;
+ u32 val;
+
+ if (fmt[1] != 'c' || fmt[2] != 'c')
+ return error_string(buf, end, "(%p4?)", spec);
+
+ if (check_pointer(&buf, end, fourcc, spec))
+ return buf;
+
+ val = *fourcc & ~BIT(31);
+
+ for (i = 0; i < sizeof(*fourcc); i++) {
+ unsigned char c = val >> (i * 8);
+
+ /* Print non-control ASCII characters as-is, dot otherwise */
+ *p++ = isascii(c) && isprint(c) ? c : '.';
+ }
+
+ strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
+ p += strlen(p);
+
+ *p++ = ' ';
+ *p++ = '(';
+ p = special_hex_number(p, output + sizeof(output) - 2, *fourcc,
sizeof(u32));
+ *p++ = ')';
+ *p = '\0';
+
+ return string(buf, end, output, spec);
+}
+
static noinline_for_stack
char *address_val(char *buf, char *end, const void *addr,
struct printf_spec spec, const char *fmt)
@@ -2162,6 +2198,7 @@ char *fwnode_string(char *buf, char *end, struct
fwnode_handle *fwnode,
* correctness of the format string and va_list arguments.
* - 'K' For a kernel pointer that should be hidden from unprivileged users
* - 'NF' For a netdev_features_t
+ * - '4cc' V4L2 or DRM FourCC code, with endianness and raw numerical value.
* - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
*a certain separator (' ' by default):
* C colon
@@ -2259,6 +2296,8 @@ char *pointer(const char *fmt, char *buf, char *end, void
*ptr,
return restricted_pointer(buf, end, ptr, spec);
case 'N':
return netdev_bits(buf, end, ptr, spec, fmt);
+ case '4':
+ return fourcc_string(buf, end, ptr, spec, fmt);
case 'a&
[PATCH v8 2/4] v4l: ioctl: Use %p4cc printk modifier to print FourCC codes
Now that we can print FourCC codes directly using printk, make use of the
feature in V4L2 core.
Signed-off-by: Sakari Ailus
Acked-by: Mauro Carvalho Chehab
Reviewed-by: Petr Mladek
Reviewed-by: Andy Shevchenko
---
drivers/media/v4l2-core/v4l2-ioctl.c | 85 +++-
1 file changed, 21 insertions(+), 64 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
b/drivers/media/v4l2-core/v4l2-ioctl.c
index 31d1342e61e8..31662c3a8c9e 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -265,13 +265,9 @@ static void v4l_print_fmtdesc(const void *arg, bool
write_only)
{
const struct v4l2_fmtdesc *p = arg;
- pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%c%c%c%c,
mbus_code=0x%04x, description='%.*s'\n",
+ pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%p4cc,
mbus_code=0x%04x, description='%.*s'\n",
p->index, prt_names(p->type, v4l2_type_names),
- p->flags, (p->pixelformat & 0xff),
- (p->pixelformat >> 8) & 0xff,
- (p->pixelformat >> 16) & 0xff,
- (p->pixelformat >> 24) & 0xff,
- p->mbus_code,
+ p->flags, &p->pixelformat, p->mbus_code,
(int)sizeof(p->description), p->description);
}
@@ -293,12 +289,8 @@ static void v4l_print_format(const void *arg, bool
write_only)
case V4L2_BUF_TYPE_VIDEO_CAPTURE:
case V4L2_BUF_TYPE_VIDEO_OUTPUT:
pix = &p->fmt.pix;
- pr_cont(", width=%u, height=%u, pixelformat=%c%c%c%c, field=%s,
bytesperline=%u, sizeimage=%u, colorspace=%d, flags=0x%x, ycbcr_enc=%u,
quantization=%u, xfer_func=%u\n",
- pix->width, pix->height,
- (pix->pixelformat & 0xff),
- (pix->pixelformat >> 8) & 0xff,
- (pix->pixelformat >> 16) & 0xff,
- (pix->pixelformat >> 24) & 0xff,
+ pr_cont(", width=%u, height=%u, pixelformat=%p4cc, field=%s,
bytesperline=%u, sizeimage=%u, colorspace=%d, flags=0x%x, ycbcr_enc=%u,
quantization=%u, xfer_func=%u\n",
+ pix->width, pix->height, &pix->pixelformat,
prt_names(pix->field, v4l2_field_names),
pix->bytesperline, pix->sizeimage,
pix->colorspace, pix->flags, pix->ycbcr_enc,
@@ -307,12 +299,8 @@ static void v4l_print_format(const void *arg, bool
write_only)
case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
mp = &p->fmt.pix_mp;
- pr_cont(", width=%u, height=%u, format=%c%c%c%c, field=%s,
colorspace=%d, num_planes=%u, flags=0x%x, ycbcr_enc=%u, quantization=%u,
xfer_func=%u\n",
- mp->width, mp->height,
- (mp->pixelformat & 0xff),
- (mp->pixelformat >> 8) & 0xff,
- (mp->pixelformat >> 16) & 0xff,
- (mp->pixelformat >> 24) & 0xff,
+ pr_cont(", width=%u, height=%u, format=%p4cc, field=%s,
colorspace=%d, num_planes=%u, flags=0x%x, ycbcr_enc=%u, quantization=%u,
xfer_func=%u\n",
+ mp->width, mp->height, &mp->pixelformat,
prt_names(mp->field, v4l2_field_names),
mp->colorspace, mp->num_planes, mp->flags,
mp->ycbcr_enc, mp->quantization, mp->xfer_func);
@@ -337,13 +325,9 @@ static void v4l_print_format(const void *arg, bool
write_only)
case V4L2_BUF_TYPE_VBI_CAPTURE:
case V4L2_BUF_TYPE_VBI_OUTPUT:
vbi = &p->fmt.vbi;
- pr_cont(", sampling_rate=%u, offset=%u, samples_per_line=%u,
sample_format=%c%c%c%c, start=%u,%u, count=%u,%u\n",
+ pr_cont(", sampling_rate=%u, offset=%u, samples_per_line=%u,
sample_format=%p4cc, start=%u,%u, count=%u,%u\n",
vbi->sampling_rate, vbi->offset,
- vbi->samples_per_line,
- (vbi->sample_format & 0xff),
- (vbi->sample_format >> 8) & 0xff,
- (vbi->sample_format >> 16) & 0xff,
- (vbi->sample_format >> 24) & 0xff,
+ vbi->samples_per_line, &vbi->sample_format,
vbi->start[0], vbi->start[1],
vbi->count[0], vbi->count[1]);
break;
@@ -360,21 +344,13 @@ static void
[PATCH v8 4/4] drm: Remove drm_get_format_name()
The %p4cc printk format modifier was recently added to print fourcc codes,
replacing drm_get_format_name(). The function is no longer needed, so
remove it.
Signed-off-by: Sakari Ailus
Reviewed-by: Petr Mladek
Reviewed-by: Andy Shevchenko
---
drivers/gpu/drm/drm_fourcc.c | 25 -
include/drm/drm_fourcc.h | 1 -
2 files changed, 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 03262472059c..5cf45aa6eedc 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -30,11 +30,6 @@
#include
#include
-static char printable_char(int c)
-{
- return isascii(c) && isprint(c) ? c : '?';
-}
-
/**
* drm_mode_legacy_fb_format - compute drm fourcc code from legacy description
* @bpp: bits per pixels
@@ -130,26 +125,6 @@ uint32_t drm_driver_legacy_fb_format(struct drm_device
*dev,
}
EXPORT_SYMBOL(drm_driver_legacy_fb_format);
-/**
- * drm_get_format_name - fill a string with a drm fourcc format's name
- * @format: format to compute name of
- * @buf: caller-supplied buffer
- */
-const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf
*buf)
-{
- snprintf(buf->str, sizeof(buf->str),
-"%c%c%c%c %s-endian (0x%08x)",
-printable_char(format & 0xff),
-printable_char((format >> 8) & 0xff),
-printable_char((format >> 16) & 0xff),
-printable_char((format >> 24) & 0x7f),
-format & DRM_FORMAT_BIG_ENDIAN ? "big" : "little",
-format);
-
- return buf->str;
-}
-EXPORT_SYMBOL(drm_get_format_name);
-
/*
* Internal function to query information for a given format. See
* drm_format_info() for the public API.
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 156b122c0ad5..3ea17b8a79d3 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -318,6 +318,5 @@ unsigned int drm_format_info_block_height(const struct
drm_format_info *info,
int plane);
uint64_t drm_format_info_min_pitch(const struct drm_format_info *info,
int plane, unsigned int buffer_width);
-const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf
*buf);
#endif /* __DRM_FOURCC_H__ */
--
2.29.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 3/4] drm: Switch to %p4cc format modifier
Switch DRM drivers from drm_get_format_name() to %p4cc. This gets rid of a
large number of temporary variables at the same time.
Signed-off-by: Sakari Ailus
Reviewed-by: Petr Mladek
Reviewed-by: Andy Shevchenko
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 5 ++---
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 5 ++---
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 5 ++---
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 5 ++---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++---
.../arm/display/komeda/komeda_format_caps.h | 11 ---
.../arm/display/komeda/komeda_framebuffer.c | 4 ++--
.../gpu/drm/arm/display/komeda/komeda_plane.c | 6 ++
drivers/gpu/drm/arm/malidp_mw.c | 7 ++-
drivers/gpu/drm/drm_atomic.c | 8 ++--
drivers/gpu/drm/drm_crtc.c| 7 ++-
drivers/gpu/drm/drm_framebuffer.c | 11 +++
drivers/gpu/drm/drm_mipi_dbi.c| 5 ++---
drivers/gpu/drm/drm_plane.c | 8 ++--
.../gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 5 ++---
drivers/gpu/drm/i915/display/intel_display.c | 14 --
.../drm/i915/display/intel_display_debugfs.c | 19 ---
drivers/gpu/drm/i915/display/intel_sprite.c | 6 ++
drivers/gpu/drm/mcde/mcde_display.c | 6 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 ++
drivers/gpu/drm/nouveau/nouveau_display.c | 9 +++--
drivers/gpu/drm/radeon/atombios_crtc.c| 10 --
drivers/gpu/drm/sun4i/sun4i_backend.c | 6 ++
drivers/gpu/drm/vkms/vkms_writeback.c | 7 ++-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 ++-
25 files changed, 64 insertions(+), 131 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index 7944781e1086..ea825b4f8ee8 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -1862,7 +1862,6 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc
*crtc,
u32 tmp, viewport_w, viewport_h;
int r;
bool bypass_lut = false;
- struct drm_format_name_buf format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -1981,8 +1980,8 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc
*crtc,
#endif
break;
default:
- DRM_ERROR("Unsupported screen format %s\n",
- drm_get_format_name(target_fb->format->format,
&format_name));
+ DRM_ERROR("Unsupported screen format %p4cc\n",
+ &target_fb->format->format);
return -EINVAL;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index 1b6ff0470011..a360a6dec198 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -1904,7 +1904,6 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc
*crtc,
u32 tmp, viewport_w, viewport_h;
int r;
bool bypass_lut = false;
- struct drm_format_name_buf format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -2023,8 +2022,8 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc
*crtc,
#endif
break;
default:
- DRM_ERROR("Unsupported screen format %s\n",
- drm_get_format_name(target_fb->format->format,
&format_name));
+ DRM_ERROR("Unsupported screen format %p4cc\n",
+ &target_fb->format->format);
return -EINVAL;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index 83a88385b762..ef124ac853b6 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
@@ -1820,7 +1820,6 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc
*crtc,
u32 viewport_w, viewport_h;
int r;
bool bypass_lut = false;
- struct drm_format_name_buf format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -1929,8 +1928,8 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc
*crtc,
#endif
break;
default:
- DRM_ERROR("Unsupported screen format %s\n",
- drm_get_format_name(target_fb->format->format,
&format_name));
+ DRM_ERROR("Unsupported screen format %p4cc\n",
+ &target_fb->format->format);
return -EINVAL;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index 224b30214427..c98650183828 100644
[PATCH v8 0/4] Add %p4cc printk modifier for V4L2 and DRM fourcc codes
Hi all,
On merging --- it would seem everyone is happy with merging this
through the drm-misc tree. The last patch should wait until all
users are gone for sure, probably to the next kernel release.
There are no users of drm_get_format_name() in linux-next
currently after the 3rd patch.
This set adds support for %p4cc printk modifier for printing V4L2 and DRM
fourcc codes. The codes are cumbersome to print manually and by adding the
modifier, this task is saved from the V4L2 and DRM frameworks as well as
related drivers. DRM actually had it handled in a way (see 3rd patch) but
the printk modifier makes printing the format easier even there. On V4L2
side it saves quite a few lines of repeating different implementations of
printing the 4cc codes.
Further work will include converting the V4L2 drivers doing the same. I
left these out from this version since individual drivers are easier
changed without dealing with multiple trees.
Since v7:
- Add more examples, one with big endian and another with a space.
- Add Y10 test format.
- Use "0123" in the size string for temporary buffer.
- Added acks.
- Split the 3rd patch into two: driver changes and removal of
drm_get_format_name().
Since v6:
- Don't drop spaces in fourcc codes.
- Print unprintable characters as dot ('.') instead of hexadecimal number
in parentheses.
- Convert DRM from drm_get_format_name() to %p4cc. I wonder if this should
be merged through the DRM tree, albeit it's probably unlikely to
conflict with other changes. Further use of the function could be a
problem.
- Make tests more realistic.
Since v5:
- Added V4L2 core conversion to %p4cc, as well as change the DRM
fourcc printing function to use %p4cc.
- Add missing checkpatch.pl checks for %p4cc modifier.
Sakari Ailus (4):
lib/vsprintf: Add support for printing V4L2 and DRM fourccs
v4l: ioctl: Use %p4cc printk modifier to print FourCC codes
drm: Switch to %p4cc format modifier
drm: Remove drm_get_format_name()
Documentation/core-api/printk-formats.rst | 18
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 5 +-
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 5 +-
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 5 +-
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 5 +-
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +-
.../arm/display/komeda/komeda_format_caps.h | 11 ---
.../arm/display/komeda/komeda_framebuffer.c | 4 +-
.../gpu/drm/arm/display/komeda/komeda_plane.c | 6 +-
drivers/gpu/drm/arm/malidp_mw.c | 7 +-
drivers/gpu/drm/drm_atomic.c | 8 +-
drivers/gpu/drm/drm_crtc.c| 7 +-
drivers/gpu/drm/drm_fourcc.c | 25 --
drivers/gpu/drm/drm_framebuffer.c | 11 +--
drivers/gpu/drm/drm_mipi_dbi.c| 5 +-
drivers/gpu/drm/drm_plane.c | 8 +-
.../gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 5 +-
drivers/gpu/drm/i915/display/intel_display.c | 14 +--
.../drm/i915/display/intel_display_debugfs.c | 19 ++---
drivers/gpu/drm/i915/display/intel_sprite.c | 6 +-
drivers/gpu/drm/mcde/mcde_display.c | 6 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 +-
drivers/gpu/drm/nouveau/nouveau_display.c | 9 +-
drivers/gpu/drm/radeon/atombios_crtc.c| 10 +--
drivers/gpu/drm/sun4i/sun4i_backend.c | 6 +-
drivers/gpu/drm/vkms/vkms_writeback.c | 7 +-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 ++--
drivers/media/v4l2-core/v4l2-ioctl.c | 85 +--
include/drm/drm_fourcc.h | 1 -
lib/test_printf.c | 18
lib/vsprintf.c| 39 +
scripts/checkpatch.pl | 6 +-
32 files changed, 164 insertions(+), 223 deletions(-)
--
2.29.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 3/3] drm: Switch to %p4cc format modifier
Hi Thomas,
Thanks for the review.
On Tue, Feb 16, 2021 at 09:37:45AM +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 15.02.21 um 12:40 schrieb Sakari Ailus:
> > Switch DRM drivers from drm_get_format_name() to %p4cc. This gets rid of a
> > large number of temporary variables at the same time.
> >
> > Signed-off-by: Sakari Ailus
> > ---
> > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 5 ++--
> > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 5 ++--
> > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 5 ++--
> > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 5 ++--
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++--
> > .../arm/display/komeda/komeda_format_caps.h | 11
> > .../arm/display/komeda/komeda_framebuffer.c | 4 +--
> > .../gpu/drm/arm/display/komeda/komeda_plane.c | 6 ++---
> > drivers/gpu/drm/arm/malidp_mw.c | 7 ++
> > drivers/gpu/drm/drm_atomic.c | 8 ++
> > drivers/gpu/drm/drm_crtc.c| 7 ++
> > drivers/gpu/drm/drm_fourcc.c | 25 ---
> > drivers/gpu/drm/drm_framebuffer.c | 11 +++-
> > drivers/gpu/drm/drm_mipi_dbi.c| 5 ++--
> > drivers/gpu/drm/drm_plane.c | 8 ++
> > .../gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 5 ++--
> > drivers/gpu/drm/i915/display/intel_display.c | 14 +++
> > .../drm/i915/display/intel_display_debugfs.c | 19 ++
> > drivers/gpu/drm/i915/display/intel_sprite.c | 6 ++---
> > drivers/gpu/drm/mcde/mcde_display.c | 6 ++---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 ++---
> > drivers/gpu/drm/nouveau/nouveau_display.c | 9 +++
> > drivers/gpu/drm/radeon/atombios_crtc.c| 10 +++-
> > drivers/gpu/drm/sun4i/sun4i_backend.c | 6 ++---
> > drivers/gpu/drm/vkms/vkms_writeback.c | 7 ++
> > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 +--
> > include/drm/drm_fourcc.h | 1 -
> > 27 files changed, 64 insertions(+), 157 deletions(-)
>
> This is a nice patchset. For the driver-related changes:
>
> Acked-by: Thomas Zimmermann
>
> But landing this patch will certainly give us build errors. There are at
> least 3 git trees involved: drm-misc-next, amd and i915. I'd expect at least
> one of them to have additional changes that still require
> drm_get_format_name().
>
> IMHO we should remove drm_get_format_name() in a later patch. Please remove
> the changes in drm_fourcc.{c,h} from this patch and maybe add a TODO comment
> to the declaration that the function is supposed to be removed.
>
> I would merge the patchset through drm-misc-next. And the final removal
> patch during the next cycle. Ok?
Sounds good. I'll split the third patch into two then.
--
Kind regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 3/3] drm: Switch to %p4cc format modifier
On Mon, Feb 15, 2021 at 03:41:14PM +0200, Andy Shevchenko wrote:
...
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > index ca41e8c00ad7..a5c9fe2e56b3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > @@ -771,21 +771,21 @@ static void intel_plane_uapi_info(struct seq_file *m,
> > struct intel_plane *plane)
> > const struct intel_plane_state *plane_state =
> > to_intel_plane_state(plane->base.state);
> > const struct drm_framebuffer *fb = plane_state->uapi.fb;
> > - struct drm_format_name_buf format_name;
> > struct drm_rect src, dst;
> > char rot_str[48];
> >
> > src = drm_plane_state_src(&plane_state->uapi);
> > dst = drm_plane_state_dest(&plane_state->uapi);
> >
> > - if (fb)
> > - drm_get_format_name(fb->format->format, &format_name);
> > -
> > plane_rotation(rot_str, sizeof(rot_str),
> >plane_state->uapi.rotation);
> >
> > - seq_printf(m, "\t\tuapi: [FB:%d] %s,0x%llx,%dx%d, visible=%s, src="
> > DRM_RECT_FP_FMT ", dst=" DRM_RECT_FMT ", rotation=%s\n",
> > - fb ? fb->base.id : 0, fb ? format_name.str : "n/a",
> > + seq_printf(m, "\t\tuapi: [FB:%d] ", fb ? fb->base.id : 0);
> > + if (fb)
> > + seq_printf(m, "%p4cc", &fb->format->format);
> > + else
> > + seq_puts(m, "n/a");
>
> > + seq_printf(m, ",0x%llx,%dx%d, visible=%s, src=" DRM_RECT_FP_FMT ",
> > dst=" DRM_RECT_FMT ", rotation=%s\n",
>
> Why not to keep two seq_printf() calls?
>
> if (fb) {
> seq_printf();
> } else {
> seq_printf();
> }
>
> ?
I could, but it'd repeat a lot of the same format string that is very
complicated right now. Therefore I thought it's better to split.
Or do you mean seq_printf() vs. seq_puts()? checkpatch.pl (rightly) warns
about it.
>
> >fb ? fb->modifier : 0,
> >fb ? fb->width : 0, fb ? fb->height : 0,
> >plane_visibility(plane_state),
--
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 2/3] v4l: ioctl: Use %p4cc printk modifier to print FourCC codes
On Mon, Feb 15, 2021 at 06:05:45PM +0100, Mauro Carvalho Chehab wrote: > Em Mon, 15 Feb 2021 17:56:27 +0100 > Petr Mladek escreveu: > > > On Mon 2021-02-15 13:40:29, Sakari Ailus wrote: > > > Now that we can print FourCC codes directly using printk, make use of the > > > feature in V4L2 core. > > > > > > Signed-off-by: Sakari Ailus > > > > Reviewed-by: Petr Mladek > > > > I am curious whether I could take this via printk tree or if Mauro > > would prefer to take this via his tree. > > IMO, the best would be if the entire series gets merged via a single > tree. > > Feel free to merge via the printk one. > > Acked-by: Mauro Carvalho Chehab Thanks, Mauro! -- Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 1/3] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Petr,
On Mon, Feb 15, 2021 at 05:51:40PM +0100, Petr Mladek wrote:
> On Mon 2021-02-15 17:54:47, Andy Shevchenko wrote:
> > On Mon, Feb 15, 2021 at 03:56:50PM +0200, Sakari Ailus wrote:
> > > On Mon, Feb 15, 2021 at 03:31:29PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Feb 15, 2021 at 01:40:28PM +0200, Sakari Ailus wrote:
> > > > > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and
> > > > > DRM
> > > > > pixel formats denoted by fourccs. The fourcc encoding is the same for
> > > > > both
> > > > > so the same implementation can be used.
> > > >
> > > > This version I almost like, feel free to add
> > > > Reviewed-by: From: Andy Shevchenko
> > > > after considering addressing below nit-picks.
> >
> > > > > +Examples::
> > > > > +
> > > > > + %p4cc BG12 little-endian (0x32314742)
> > > >
> > > > No examples with spaces / non-printable / non-ascii characters
> > >
> > > I can sure add an example that has a space. But do you think I really
> > > should add an example where invalid information is being printed?
> >
> > I think you have to provide better coverage of what user can get out of
> > this.
> > Perhaps one example with space and non-printable character is enough.
> >
> > > > > + char output[sizeof("1234 little-endian (0x01234567)")];
> > > >
> > > > 1234 -> ABCD ? (Or XY12 to be closer to the reality)
> > >
> > > I count in numbers... albeit the hexadecimal number there starts from
> > > zero.
> > >
> > > I guess both would work though.
> > >
> > > 0123 would be consistent.
> >
> > Since letters can be printed the above is confusing a bit. I think XY12 is
> > closer to the reality than 0123.
>
> Ailus, are you going to send v8 with the two small changes? I mean a
> selftest with the space and the above sample code.
Yes, and a few more examples.
>
> Anyway, feel free to add:
>
> Reviewed-by: Petr Mladek
Thank you.
It'd be great if we could merge this through the printk tree. Acks are
needed from the DRM people first.
--
Regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 1/3] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Andy,
On Mon, Feb 15, 2021 at 03:31:29PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 15, 2021 at 01:40:28PM +0200, Sakari Ailus wrote:
> > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
> > pixel formats denoted by fourccs. The fourcc encoding is the same for both
> > so the same implementation can be used.
>
> This version I almost like, feel free to add
> Reviewed-by: From: Andy Shevchenko
> after considering addressing below nit-picks.
>
> > Suggested-by: Mauro Carvalho Chehab
> > Signed-off-by: Sakari Ailus
> > Reviewed-by: Petr Mladek
> > Reviewed-by: Sergey Senozhatsky
> > ---
> > Documentation/core-api/printk-formats.rst | 16 ++
> > lib/test_printf.c | 17 ++
> > lib/vsprintf.c| 39 +++
> > scripts/checkpatch.pl | 6 ++--
> > 4 files changed, 76 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/core-api/printk-formats.rst
> > b/Documentation/core-api/printk-formats.rst
> > index 160e710d992f..da2aa065dc42 100644
> > --- a/Documentation/core-api/printk-formats.rst
> > +++ b/Documentation/core-api/printk-formats.rst
> > @@ -567,6 +567,22 @@ For printing netdev_features_t.
> >
> > Passed by reference.
> >
> > +V4L2 and DRM FourCC code (pixel format)
> > +---
> > +
> > +::
> > +
> > + %p4cc
> > +
> > +Print a FourCC code used by V4L2 or DRM, including format endianness and
> > +its numerical value as hexadecimal.
> > +
> > +Passed by reference.
> > +
> > +Examples::
> > +
> > + %p4cc BG12 little-endian (0x32314742)
>
> No examples with spaces / non-printable / non-ascii characters
I can sure add an example that has a space. But do you think I really
should add an example where invalid information is being printed?
>
> > +
> > Thanks
> > ==
> >
> > diff --git a/lib/test_printf.c b/lib/test_printf.c
> > index 7d60f24240a4..9848510a2786 100644
> > --- a/lib/test_printf.c
> > +++ b/lib/test_printf.c
> > @@ -647,6 +647,22 @@ static void __init fwnode_pointer(void)
> > software_node_unregister_nodes(softnodes);
> > }
> >
> > +static void __init fourcc_pointer(void)
> > +{
> > + struct {
> > + u32 code;
> > + char *str;
> > + } const try[] = {
> > + { 0x3231564e, "NV12 little-endian (0x3231564e)", },
> > + { 0xb231564e, "NV12 big-endian (0xb231564e)", },
> > + { 0x10111213, " little-endian (0x10111213)", },
> > + };
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(try); i++)
> > + test(try[i].str, "%p4cc", &try[i].code);
> > +}
> > +
> > static void __init
> > errptr(void)
> > {
> > @@ -692,6 +708,7 @@ test_pointer(void)
> > flags();
> > errptr();
> > fwnode_pointer();
> > + fourcc_pointer();
> > }
> >
> > static void __init selftest(void)
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 3b53c73580c5..432b5a2d1e90 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1733,6 +1733,42 @@ char *netdev_bits(char *buf, char *end, const void
> > *addr,
> > return special_hex_number(buf, end, num, size);
> > }
> >
> > +static noinline_for_stack
> > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > + struct printf_spec spec, const char *fmt)
> > +{
> > + char output[sizeof("1234 little-endian (0x01234567)")];
>
> 1234 -> ABCD ? (Or XY12 to be closer to the reality)
I count in numbers... albeit the hexadecimal number there starts from zero.
I guess both would work though.
0123 would be consistent.
--
Regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 2/3] v4l: ioctl: Use %p4cc printk modifier to print FourCC codes
Hi Andy,
On Mon, Feb 15, 2021 at 03:34:04PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 15, 2021 at 01:40:29PM +0200, Sakari Ailus wrote:
> > Now that we can print FourCC codes directly using printk, make use of the
> > feature in V4L2 core.
>
> Reviewed-by: Andy Shevchenko
> See also below.
Thanks for the review, and the tag!
>
> > Signed-off-by: Sakari Ailus
> > ---
> > drivers/media/v4l2-core/v4l2-ioctl.c | 85 +++-
> > 1 file changed, 21 insertions(+), 64 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> > b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index 31d1342e61e8..31662c3a8c9e 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -265,13 +265,9 @@ static void v4l_print_fmtdesc(const void *arg, bool
> > write_only)
> > {
> > const struct v4l2_fmtdesc *p = arg;
> >
> > - pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%c%c%c%c,
> > mbus_code=0x%04x, description='%.*s'\n",
> > + pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%p4cc,
> > mbus_code=0x%04x, description='%.*s'\n",
> > p->index, prt_names(p->type, v4l2_type_names),
> > - p->flags, (p->pixelformat & 0xff),
> > - (p->pixelformat >> 8) & 0xff,
> > - (p->pixelformat >> 16) & 0xff,
> > - (p->pixelformat >> 24) & 0xff,
> > - p->mbus_code,
> > + p->flags, &p->pixelformat, p->mbus_code,
> > (int)sizeof(p->description), p->description);
> > }
> >
> > @@ -293,12 +289,8 @@ static void v4l_print_format(const void *arg, bool
> > write_only)
> > case V4L2_BUF_TYPE_VIDEO_CAPTURE:
> > case V4L2_BUF_TYPE_VIDEO_OUTPUT:
> > pix = &p->fmt.pix;
> > - pr_cont(", width=%u, height=%u, pixelformat=%c%c%c%c, field=%s,
> > bytesperline=%u, sizeimage=%u, colorspace=%d, flags=0x%x, ycbcr_enc=%u,
> > quantization=%u, xfer_func=%u\n",
> > - pix->width, pix->height,
> > - (pix->pixelformat & 0xff),
> > - (pix->pixelformat >> 8) & 0xff,
> > - (pix->pixelformat >> 16) & 0xff,
> > - (pix->pixelformat >> 24) & 0xff,
> > + pr_cont(", width=%u, height=%u, pixelformat=%p4cc, field=%s,
> > bytesperline=%u, sizeimage=%u, colorspace=%d, flags=0x%x, ycbcr_enc=%u,
> > quantization=%u, xfer_func=%u\n",
> > + pix->width, pix->height, &pix->pixelformat,
> > prt_names(pix->field, v4l2_field_names),
> > pix->bytesperline, pix->sizeimage,
> > pix->colorspace, pix->flags, pix->ycbcr_enc,
> > @@ -307,12 +299,8 @@ static void v4l_print_format(const void *arg, bool
> > write_only)
> > case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > mp = &p->fmt.pix_mp;
> > - pr_cont(", width=%u, height=%u, format=%c%c%c%c, field=%s,
> > colorspace=%d, num_planes=%u, flags=0x%x, ycbcr_enc=%u, quantization=%u,
> > xfer_func=%u\n",
> > - mp->width, mp->height,
> > - (mp->pixelformat & 0xff),
> > - (mp->pixelformat >> 8) & 0xff,
> > - (mp->pixelformat >> 16) & 0xff,
> > - (mp->pixelformat >> 24) & 0xff,
> > + pr_cont(", width=%u, height=%u, format=%p4cc, field=%s,
> > colorspace=%d, num_planes=%u, flags=0x%x, ycbcr_enc=%u, quantization=%u,
> > xfer_func=%u\n",
> > + mp->width, mp->height, &mp->pixelformat,
> > prt_names(mp->field, v4l2_field_names),
> > mp->colorspace, mp->num_planes, mp->flags,
> > mp->ycbcr_enc, mp->quantization, mp->xfer_func);
> > @@ -337,13 +325,9 @@ static void v4l_print_format(const void *arg, bool
> > write_only)
> > case V4L2_BUF_TYPE_VBI_CAPTURE:
> > case V4L2_BUF_TYPE_VBI_OUTPUT:
> > vbi = &p->fmt.vbi;
> > - pr_cont(", sampling_rate=%u, offset=%u, samples_per_line=%u,
> > sample_format=%c%c%c%c, start=%u,%u, count=%u,%u\n",
> > + p
Re: [PATCH v7 3/3] drm: Switch to %p4cc format modifier
Hi Andy,
On Mon, Feb 15, 2021 at 03:41:14PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 15, 2021 at 01:40:30PM +0200, Sakari Ailus wrote:
> > Switch DRM drivers from drm_get_format_name() to %p4cc. This gets rid of a
> > large number of temporary variables at the same time.
>
> What a nice clean up!
> Reviewed-by: Andy Shevchenko
Thanks!
> after addressing nit-picks below.
>
> Side note (no need to implement esp. right now): it seems often it's coupled
> with modifier, would be nice to have them together that %p4ccM or so can do it
> in one go.
...
> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > index 03262472059c..5cf45aa6eedc 100644
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -30,11 +30,6 @@
> > #include
> > #include
> >
> > -static char printable_char(int c)
> > -{
> > - return isascii(c) && isprint(c) ? c : '?';
> > -}
>
> If it goes as an ABI than your dot is incompatible with this and '?' should be
> used instead in the patch 1. And I bend towards suggested '?' rather than '.'.
>
> Also it means that you probably would need different specifiers for full and
> short formats.
I thought of that, but then the resulting string would be indeed different
and comparing short and long formats would be harder. Remember this is for
debug prints. If the format is too long, then it should be made shorter,
but during the earlier review rounds people have expressed interest in
having this information there.
"?" can be expanded by the shell whereas "." is not. If DRM folks think we
should go back to "?" I'm fine with that. Also note that there's something
wrong with the fourcc code to begin with if it's got either "." or "?".
--
Regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v7 2/3] v4l: ioctl: Use %p4cc printk modifier to print FourCC codes
Now that we can print FourCC codes directly using printk, make use of the
feature in V4L2 core.
Signed-off-by: Sakari Ailus
---
drivers/media/v4l2-core/v4l2-ioctl.c | 85 +++-
1 file changed, 21 insertions(+), 64 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
b/drivers/media/v4l2-core/v4l2-ioctl.c
index 31d1342e61e8..31662c3a8c9e 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -265,13 +265,9 @@ static void v4l_print_fmtdesc(const void *arg, bool
write_only)
{
const struct v4l2_fmtdesc *p = arg;
- pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%c%c%c%c,
mbus_code=0x%04x, description='%.*s'\n",
+ pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%p4cc,
mbus_code=0x%04x, description='%.*s'\n",
p->index, prt_names(p->type, v4l2_type_names),
- p->flags, (p->pixelformat & 0xff),
- (p->pixelformat >> 8) & 0xff,
- (p->pixelformat >> 16) & 0xff,
- (p->pixelformat >> 24) & 0xff,
- p->mbus_code,
+ p->flags, &p->pixelformat, p->mbus_code,
(int)sizeof(p->description), p->description);
}
@@ -293,12 +289,8 @@ static void v4l_print_format(const void *arg, bool
write_only)
case V4L2_BUF_TYPE_VIDEO_CAPTURE:
case V4L2_BUF_TYPE_VIDEO_OUTPUT:
pix = &p->fmt.pix;
- pr_cont(", width=%u, height=%u, pixelformat=%c%c%c%c, field=%s,
bytesperline=%u, sizeimage=%u, colorspace=%d, flags=0x%x, ycbcr_enc=%u,
quantization=%u, xfer_func=%u\n",
- pix->width, pix->height,
- (pix->pixelformat & 0xff),
- (pix->pixelformat >> 8) & 0xff,
- (pix->pixelformat >> 16) & 0xff,
- (pix->pixelformat >> 24) & 0xff,
+ pr_cont(", width=%u, height=%u, pixelformat=%p4cc, field=%s,
bytesperline=%u, sizeimage=%u, colorspace=%d, flags=0x%x, ycbcr_enc=%u,
quantization=%u, xfer_func=%u\n",
+ pix->width, pix->height, &pix->pixelformat,
prt_names(pix->field, v4l2_field_names),
pix->bytesperline, pix->sizeimage,
pix->colorspace, pix->flags, pix->ycbcr_enc,
@@ -307,12 +299,8 @@ static void v4l_print_format(const void *arg, bool
write_only)
case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
mp = &p->fmt.pix_mp;
- pr_cont(", width=%u, height=%u, format=%c%c%c%c, field=%s,
colorspace=%d, num_planes=%u, flags=0x%x, ycbcr_enc=%u, quantization=%u,
xfer_func=%u\n",
- mp->width, mp->height,
- (mp->pixelformat & 0xff),
- (mp->pixelformat >> 8) & 0xff,
- (mp->pixelformat >> 16) & 0xff,
- (mp->pixelformat >> 24) & 0xff,
+ pr_cont(", width=%u, height=%u, format=%p4cc, field=%s,
colorspace=%d, num_planes=%u, flags=0x%x, ycbcr_enc=%u, quantization=%u,
xfer_func=%u\n",
+ mp->width, mp->height, &mp->pixelformat,
prt_names(mp->field, v4l2_field_names),
mp->colorspace, mp->num_planes, mp->flags,
mp->ycbcr_enc, mp->quantization, mp->xfer_func);
@@ -337,13 +325,9 @@ static void v4l_print_format(const void *arg, bool
write_only)
case V4L2_BUF_TYPE_VBI_CAPTURE:
case V4L2_BUF_TYPE_VBI_OUTPUT:
vbi = &p->fmt.vbi;
- pr_cont(", sampling_rate=%u, offset=%u, samples_per_line=%u,
sample_format=%c%c%c%c, start=%u,%u, count=%u,%u\n",
+ pr_cont(", sampling_rate=%u, offset=%u, samples_per_line=%u,
sample_format=%p4cc, start=%u,%u, count=%u,%u\n",
vbi->sampling_rate, vbi->offset,
- vbi->samples_per_line,
- (vbi->sample_format & 0xff),
- (vbi->sample_format >> 8) & 0xff,
- (vbi->sample_format >> 16) & 0xff,
- (vbi->sample_format >> 24) & 0xff,
+ vbi->samples_per_line, &vbi->sample_format,
vbi->start[0], vbi->start[1],
vbi->count[0], vbi->count[1]);
break;
@@ -360,21 +344,13 @@ static void v4l_print_format(const void *arg, bool
write_only)
case V4L2_BUF_TYPE_SDR_CAPTURE:
[PATCH v7 3/3] drm: Switch to %p4cc format modifier
Switch DRM drivers from drm_get_format_name() to %p4cc. This gets rid of a
large number of temporary variables at the same time.
Signed-off-by: Sakari Ailus
---
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 5 ++--
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 5 ++--
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 5 ++--
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 5 ++--
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++--
.../arm/display/komeda/komeda_format_caps.h | 11
.../arm/display/komeda/komeda_framebuffer.c | 4 +--
.../gpu/drm/arm/display/komeda/komeda_plane.c | 6 ++---
drivers/gpu/drm/arm/malidp_mw.c | 7 ++
drivers/gpu/drm/drm_atomic.c | 8 ++
drivers/gpu/drm/drm_crtc.c| 7 ++
drivers/gpu/drm/drm_fourcc.c | 25 ---
drivers/gpu/drm/drm_framebuffer.c | 11 +++-
drivers/gpu/drm/drm_mipi_dbi.c| 5 ++--
drivers/gpu/drm/drm_plane.c | 8 ++
.../gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 5 ++--
drivers/gpu/drm/i915/display/intel_display.c | 14 +++
.../drm/i915/display/intel_display_debugfs.c | 19 ++
drivers/gpu/drm/i915/display/intel_sprite.c | 6 ++---
drivers/gpu/drm/mcde/mcde_display.c | 6 ++---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 ++---
drivers/gpu/drm/nouveau/nouveau_display.c | 9 +++
drivers/gpu/drm/radeon/atombios_crtc.c| 10 +++-
drivers/gpu/drm/sun4i/sun4i_backend.c | 6 ++---
drivers/gpu/drm/vkms/vkms_writeback.c | 7 ++
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 +--
include/drm/drm_fourcc.h | 1 -
27 files changed, 64 insertions(+), 157 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index 7944781e1086..ea825b4f8ee8 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -1862,7 +1862,6 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc
*crtc,
u32 tmp, viewport_w, viewport_h;
int r;
bool bypass_lut = false;
- struct drm_format_name_buf format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -1981,8 +1980,8 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc
*crtc,
#endif
break;
default:
- DRM_ERROR("Unsupported screen format %s\n",
- drm_get_format_name(target_fb->format->format,
&format_name));
+ DRM_ERROR("Unsupported screen format %p4cc\n",
+ &target_fb->format->format);
return -EINVAL;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index 1b6ff0470011..a360a6dec198 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -1904,7 +1904,6 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc
*crtc,
u32 tmp, viewport_w, viewport_h;
int r;
bool bypass_lut = false;
- struct drm_format_name_buf format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -2023,8 +2022,8 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc
*crtc,
#endif
break;
default:
- DRM_ERROR("Unsupported screen format %s\n",
- drm_get_format_name(target_fb->format->format,
&format_name));
+ DRM_ERROR("Unsupported screen format %p4cc\n",
+ &target_fb->format->format);
return -EINVAL;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index 83a88385b762..ef124ac853b6 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
@@ -1820,7 +1820,6 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc
*crtc,
u32 viewport_w, viewport_h;
int r;
bool bypass_lut = false;
- struct drm_format_name_buf format_name;
/* no fb bound */
if (!atomic && !crtc->primary->fb) {
@@ -1929,8 +1928,8 @@ static int dce_v6_0_crtc_do_set_base(struct drm_crtc
*crtc,
#endif
break;
default:
- DRM_ERROR("Unsupported screen format %s\n",
- drm_get_format_name(target_fb->format->format,
&format_name));
+ DRM_ERROR("Unsupported screen format %p4cc\n",
+ &target_fb->format->format);
return -EINVAL;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index 224b30214427..c98650183828 100644
--- a/d
[PATCH v7 1/3] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
pixel formats denoted by fourccs. The fourcc encoding is the same for both
so the same implementation can be used.
Suggested-by: Mauro Carvalho Chehab
Signed-off-by: Sakari Ailus
Reviewed-by: Petr Mladek
Reviewed-by: Sergey Senozhatsky
---
Documentation/core-api/printk-formats.rst | 16 ++
lib/test_printf.c | 17 ++
lib/vsprintf.c| 39 +++
scripts/checkpatch.pl | 6 ++--
4 files changed, 76 insertions(+), 2 deletions(-)
diff --git a/Documentation/core-api/printk-formats.rst
b/Documentation/core-api/printk-formats.rst
index 160e710d992f..da2aa065dc42 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -567,6 +567,22 @@ For printing netdev_features_t.
Passed by reference.
+V4L2 and DRM FourCC code (pixel format)
+---
+
+::
+
+ %p4cc
+
+Print a FourCC code used by V4L2 or DRM, including format endianness and
+its numerical value as hexadecimal.
+
+Passed by reference.
+
+Examples::
+
+ %p4cc BG12 little-endian (0x32314742)
+
Thanks
==
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7d60f24240a4..9848510a2786 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -647,6 +647,22 @@ static void __init fwnode_pointer(void)
software_node_unregister_nodes(softnodes);
}
+static void __init fourcc_pointer(void)
+{
+ struct {
+ u32 code;
+ char *str;
+ } const try[] = {
+ { 0x3231564e, "NV12 little-endian (0x3231564e)", },
+ { 0xb231564e, "NV12 big-endian (0xb231564e)", },
+ { 0x10111213, " little-endian (0x10111213)", },
+ };
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(try); i++)
+ test(try[i].str, "%p4cc", &try[i].code);
+}
+
static void __init
errptr(void)
{
@@ -692,6 +708,7 @@ test_pointer(void)
flags();
errptr();
fwnode_pointer();
+ fourcc_pointer();
}
static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..432b5a2d1e90 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1733,6 +1733,42 @@ char *netdev_bits(char *buf, char *end, const void *addr,
return special_hex_number(buf, end, num, size);
}
+static noinline_for_stack
+char *fourcc_string(char *buf, char *end, const u32 *fourcc,
+ struct printf_spec spec, const char *fmt)
+{
+ char output[sizeof("1234 little-endian (0x01234567)")];
+ char *p = output;
+ unsigned int i;
+ u32 val;
+
+ if (fmt[1] != 'c' || fmt[2] != 'c')
+ return error_string(buf, end, "(%p4?)", spec);
+
+ if (check_pointer(&buf, end, fourcc, spec))
+ return buf;
+
+ val = *fourcc & ~BIT(31);
+
+ for (i = 0; i < sizeof(*fourcc); i++) {
+ unsigned char c = val >> (i * 8);
+
+ /* Print non-control ASCII characters as-is, dot otherwise */
+ *p++ = isascii(c) && isprint(c) ? c : '.';
+ }
+
+ strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
+ p += strlen(p);
+
+ *p++ = ' ';
+ *p++ = '(';
+ p = special_hex_number(p, output + sizeof(output) - 2, *fourcc,
sizeof(u32));
+ *p++ = ')';
+ *p = '\0';
+
+ return string(buf, end, output, spec);
+}
+
static noinline_for_stack
char *address_val(char *buf, char *end, const void *addr,
struct printf_spec spec, const char *fmt)
@@ -2162,6 +2198,7 @@ char *fwnode_string(char *buf, char *end, struct
fwnode_handle *fwnode,
* correctness of the format string and va_list arguments.
* - 'K' For a kernel pointer that should be hidden from unprivileged users
* - 'NF' For a netdev_features_t
+ * - '4cc' V4L2 or DRM FourCC code, with endianness and raw numerical value.
* - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
*a certain separator (' ' by default):
* C colon
@@ -2259,6 +2296,8 @@ char *pointer(const char *fmt, char *buf, char *end, void
*ptr,
return restricted_pointer(buf, end, ptr, spec);
case 'N':
return netdev_bits(buf, end, ptr, spec, fmt);
+ case '4':
+ return fourcc_string(buf, end, ptr, spec, fmt);
case 'a':
return address_val(buf, end, ptr, spec, fmt);
case 'd':
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 92e888ed939f..79858e
[PATCH v7 0/3] Add %p4cc printk modifier for V4L2 and DRM fourcc codes
Hi all,
This set adds support for %p4cc printk modifier for printing V4L2 and DRM
fourcc codes. The codes are cumbersome to print manually and by adding the
modifier, this task is saved from the V4L2 and DRM frameworks as well as
related drivers. DRM actually had it handled in a way (see 3rd patch) but
the printk modifier makes printing the format easier even there. On V4L2
side it saves quite a few lines of repeating different implementations of
printing the 4cc codes.
Further work will include converting the V4L2 drivers doing the same, as
well as converting DRM drivers from drm_get_format_name() to plain %p4cc.
I left these out from this version since individual drivers are easier
changed without dealing with multiple trees.
If DRM folks would prefer to convert drivers to %p4cc directly instead I
have no problem dropping the 3rd patch. Nearly all uses in DRM are in
printk family of functions that can readily use %p4cc instead of the
current arrangement that relies on caller-allocated temporary buffer.
Since v6:
- Don't drop spaces in fourcc codes.
- Print unprintable characters as dot ('.') instead of hexadecimal number
in parentheses.
- Convert DRM from drm_get_format_name() to %p4cc. I wonder if this should
be merged through the DRM tree, albeit it's probably unlikely to
conflict with other changes. Further use of the function could be a
problem.
- Make tests more realistic.
Since v5:
- Added V4L2 core conversion to %p4cc, as well as change the DRM
fourcc printing function to use %p4cc.
- Add missing checkpatch.pl checks for %p4cc modifier.
Sakari Ailus (3):
lib/vsprintf: Add support for printing V4L2 and DRM fourccs
v4l: ioctl: Use %p4cc printk modifier to print FourCC codes
drm: Switch to %p4cc format modifier
Documentation/core-api/printk-formats.rst | 16
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 5 +-
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 5 +-
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 5 +-
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 5 +-
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +-
.../arm/display/komeda/komeda_format_caps.h | 11 ---
.../arm/display/komeda/komeda_framebuffer.c | 4 +-
.../gpu/drm/arm/display/komeda/komeda_plane.c | 6 +-
drivers/gpu/drm/arm/malidp_mw.c | 7 +-
drivers/gpu/drm/drm_atomic.c | 8 +-
drivers/gpu/drm/drm_crtc.c| 7 +-
drivers/gpu/drm/drm_fourcc.c | 25 --
drivers/gpu/drm/drm_framebuffer.c | 11 +--
drivers/gpu/drm/drm_mipi_dbi.c| 5 +-
drivers/gpu/drm/drm_plane.c | 8 +-
.../gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 5 +-
drivers/gpu/drm/i915/display/intel_display.c | 14 +--
.../drm/i915/display/intel_display_debugfs.c | 19 ++---
drivers/gpu/drm/i915/display/intel_sprite.c | 6 +-
drivers/gpu/drm/mcde/mcde_display.c | 6 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 +-
drivers/gpu/drm/nouveau/nouveau_display.c | 9 +-
drivers/gpu/drm/radeon/atombios_crtc.c| 10 +--
drivers/gpu/drm/sun4i/sun4i_backend.c | 6 +-
drivers/gpu/drm/vkms/vkms_writeback.c | 7 +-
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 ++--
drivers/media/v4l2-core/v4l2-ioctl.c | 85 +--
include/drm/drm_fourcc.h | 1 -
lib/test_printf.c | 17
lib/vsprintf.c| 39 +
scripts/checkpatch.pl | 6 +-
32 files changed, 161 insertions(+), 223 deletions(-)
--
2.29.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 1/3] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Petr,
Thanks for the comments.
On Thu, Feb 11, 2021 at 06:14:28PM +0100, Petr Mladek wrote:
> On Tue 2021-02-09 19:47:55, Sakari Ailus wrote:
> > Hi Andy,
> >
> > On Tue, Feb 09, 2021 at 11:58:40AM +0200, Andy Shevchenko wrote:
> > > On Tue, Feb 09, 2021 at 11:20:32AM +0200, Sakari Ailus wrote:
> > > > On Mon, Feb 08, 2021 at 10:43:30PM +0200, Andy Shevchenko wrote:
> > > > > On Mon, Feb 8, 2021 at 10:11 PM Sakari Ailus
> > > > > wrote:
> > >
> > > ...
> > >
> > > > > > + %p4cc BG12 little-endian (0x32314742)
> > > > >
> > > > > This misses examples of the (strange) escaping cases and wiped
> > > > > whitespaces to make sure everybody understands that 'D 12' will be the
> > > > > same as 'D1 2' (side note: which I disagree on, perhaps something
> > > > > should be added into documentation why).
> > > >
> > > > The spaces are expected to be at the end only. I can add such example if
> > > > you like. There are no fourcc codes with spaces in the middle in neither
> > > > V4L2 nor DRM, and I don't think the expectation is to have them either.
> > >
> > > But then the code suggests otherwise. Perhaps we need to extract
> > > skip_trailing_spaces() from strim() and use it here.
> >
> > But this wouldn't affect the result in this case, would it?
>
> Is there any existing implementation that would skip spaces, please?
>
> IMHO, this might just hide problems. We should show exactly what
> is stored unless anyone explicitly ask for skipping that spaces.
I discussed this with Hans and we concluded spaces shouldn't be dropped.
Spaces can be present in the codes themselves in any case.
>
> > >
> > > ...
> > >
> > > > > > +static noinline_for_stack
> > > > > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > > > > + struct printf_spec spec, const char *fmt)
> > > > > > +{
> > > > >
> > > > > > + char output[sizeof("(xx)(xx)(xx)(xx) little-endian
> > > > > > (0x01234567)")];
> > > > >
> > > > > Do we have any evidence / document / standard that the above format is
> > > > > what people would find good? From existing practices (I consider other
> > > > > printings elsewhere and users in this series) I find '(xx)' form for
> > > > > hex numbers is weird. The standard practice is to use \xHH (without
> > > > > parentheses).
> > > >
> > > > Earlier in the review it was proposed that special handling of codes
> > > > below
> > > > 32 should be added, which I did. Using the parentheses is apparently an
> > > > existing practice elsewhere.
> > >
> > > Where? \xHH is quite well established format for escaping. Never heard
> > > about
> > > '(xx)' variant before this very series.
>
> > Mauro referred to FourCC codes while reviewing an earlier version of this,
> > such as RGB(15).
>
> This is quite easy to parse. The problem is that it is not clear
> whether it is hexa or decimal number.
>
> > Does \× imply only the next two characters are hexadecimal? I have to admit
> > I don't remember seeting that, nor \x notation is common.
>
> Hmm, the /xyy format might be hard to parse.
>
> What about using the same approach as drm_get_format_name()?
> I mean printing '?' when the character is not printable.
> The exact value is printed later anyway.
>
> The advantage is that it will always printk 4 characters.
"?" can be expanded by the shell. We (me and Hans) both though a dot (".")
would be good. These aren't going to be present in valid fourcc codes in
any case.
>
>
> > > > Note that neither DRM nor V4L2 currently has such fourcc codes
> > > > currently.
> > >
> > > ...
> > >
> > > > > > + p = special_hex_number(p, output + sizeof(output) - 2,
> > > > > > *fourcc,
> > > > > > + sizeof(u32));
> > > > >
> > > > > This is perfectly one line (in this file we have even longer lines).
>
> Ailus, please do not take this as a criticism of your patch.
> I understand that it might have sounded like this but Andy did
> not mean it.
>
> Andy prefers slightly longer lines over wrapping only few characters.
> It makes sense to me. There are more people with the same opinion.
> Even checkpatch.pl tolerates lines up to 100 characters these days.
>
> Of course, this is a subsystem specific preference. You did not have
> any chance to know it. There is no need to fight over it.
Fair enough; I can violate the coding style a little in v7.
--
Kind regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 2/3] v4l: ioctl: Use %p4cc printk modifier to print FourCC codes
Hi Petr,
On Thu, Feb 11, 2021 at 05:31:46PM +0100, Petr Mladek wrote:
> On Mon 2021-02-08 22:09:02, Sakari Ailus wrote:
> > Now that we can print FourCC codes directly using printk, make use of the
> > feature in V4L2 core.
> >
> > Signed-off-by: Sakari Ailus
> > ---
> > drivers/media/v4l2-core/v4l2-ioctl.c | 85 +++-
> > 1 file changed, 21 insertions(+), 64 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> > b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index 31d1342e61e8..31662c3a8c9e 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -265,13 +265,9 @@ static void v4l_print_fmtdesc(const void *arg, bool
> > write_only)
> > {
> > const struct v4l2_fmtdesc *p = arg;
> >
> > - pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%c%c%c%c,
> > mbus_code=0x%04x, description='%.*s'\n",
> > + pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%p4cc,
> > mbus_code=0x%04x, description='%.*s'\n",
>
> Is %p4cc really acceptable here?
>
> The original code printed only the 4 characters. The original code
> would print something like:
>
> index=21, type=bla, flags=0x0, pixelformat=BG12, mbus_code=0x0a9f,
> descrition="bla bla bla"
>
> while the new code will do:
>
> index=21, type=bla, flags=0x0, pixelformat=BG12 little-endian (0x32314742),
> mbus_code=0x0a9f, descrition="bla bla bla"
>
> This is much harder to parse because there are spaces also inside
> pixel_format=
Note that also the fourcc code itself could contains spaces so that's not
new.
The fourcc (debug) form is now one and the same for V4L2 and DRM, but I
guess nothing would prevent adding a shorter form if needed. This is not
continuously happening during streaming so this is also not performance
critical in any way. The fourcc code was used to be printed this way here
mainly because it was, well, easy to do for "just" debugging purposes.
Hans, any opinion?
--
Kind regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 1/3] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Andy,
On Tue, Feb 09, 2021 at 11:58:40AM +0200, Andy Shevchenko wrote:
> On Tue, Feb 09, 2021 at 11:20:32AM +0200, Sakari Ailus wrote:
> > On Mon, Feb 08, 2021 at 10:43:30PM +0200, Andy Shevchenko wrote:
> > > On Mon, Feb 8, 2021 at 10:11 PM Sakari Ailus
> > > wrote:
>
> ...
>
> > > > + %p4cc BG12 little-endian (0x32314742)
> > >
> > > This misses examples of the (strange) escaping cases and wiped
> > > whitespaces to make sure everybody understands that 'D 12' will be the
> > > same as 'D1 2' (side note: which I disagree on, perhaps something
> > > should be added into documentation why).
> >
> > The spaces are expected to be at the end only. I can add such example if
> > you like. There are no fourcc codes with spaces in the middle in neither
> > V4L2 nor DRM, and I don't think the expectation is to have them either.
>
> But then the code suggests otherwise. Perhaps we need to extract
> skip_trailing_spaces() from strim() and use it here.
But this wouldn't affect the result in this case, would it?
>
> ...
>
> > > > +static noinline_for_stack
> > > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > > + struct printf_spec spec, const char *fmt)
> > > > +{
> > >
> > > > + char output[sizeof("(xx)(xx)(xx)(xx) little-endian
> > > > (0x01234567)")];
> > >
> > > Do we have any evidence / document / standard that the above format is
> > > what people would find good? From existing practices (I consider other
> > > printings elsewhere and users in this series) I find '(xx)' form for
> > > hex numbers is weird. The standard practice is to use \xHH (without
> > > parentheses).
> >
> > Earlier in the review it was proposed that special handling of codes below
> > 32 should be added, which I did. Using the parentheses is apparently an
> > existing practice elsewhere.
>
> Where? \xHH is quite well established format for escaping. Never heard about
> '(xx)' variant before this very series.
Mauro referred to FourCC codes while reviewing an earlier version of this,
such as RGB(15).
Does \× imply only the next two characters are hexadecimal? I have to admit
I don't remember seeting that, nor \x notation is common.
>
> > Note that neither DRM nor V4L2 currently has such fourcc codes currently.
>
> ...
>
> > > > + p = special_hex_number(p, output + sizeof(output) - 2, *fourcc,
> > > > + sizeof(u32));
> > >
> > > This is perfectly one line (in this file we have even longer lines).
> >
> > Sure, you can do that, and I can then review your patch and point to the
> > coding style documentation. :-)
>
> Yes, you can. The problem is that we agreed with others to improve readability
> by letting some lines to be longer, so the code can lie on one line rather be
> broken on two or more.
Making the end of the line invisible without scrolling vertically doesn't
improve readability for me. It does depend on window width though. I'd
simply only use that if 80 isn't enough.
--
Regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 1/3] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Andy,
On Mon, Feb 08, 2021 at 10:43:30PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 8, 2021 at 10:11 PM Sakari Ailus
> wrote:
> >
> > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
> > pixel formats denoted by fourccs. The fourcc encoding is the same for both
> > so the same implementation can be used.
>
> Thank you for an update with the examples how current users will be
> converted. Below review is based on the users I had seen so far and
> assumptions made in this code. I see that it's tagged by maintainers,
> but I can't help to comment again on this. In any case the decision is
> up to them.
>
> ...
>
> > +V4L2 and DRM FourCC code (pixel format)
> > +---
> > +
> > +::
> > +
> > + %p4cc
> > +
> > +Print a FourCC code used by V4L2 or DRM, including format endianness and
> > +its numerical value as hexadecimal.
> > +
> > +Passed by reference.
> > +
> > +Examples::
> > +
> > + %p4cc BG12 little-endian (0x32314742)
>
> This misses examples of the (strange) escaping cases and wiped
> whitespaces to make sure everybody understands that 'D 12' will be the
> same as 'D1 2' (side note: which I disagree on, perhaps something
> should be added into documentation why).
The spaces are expected to be at the end only. I can add such example if
you like. There are no fourcc codes with spaces in the middle in neither
V4L2 nor DRM, and I don't think the expectation is to have them either.
>
> ...
>
> > +static noinline_for_stack
> > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > + struct printf_spec spec, const char *fmt)
> > +{
>
> > + char output[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")];
>
> Do we have any evidence / document / standard that the above format is
> what people would find good? From existing practices (I consider other
> printings elsewhere and users in this series) I find '(xx)' form for
> hex numbers is weird. The standard practice is to use \xHH (without
> parentheses).
Earlier in the review it was proposed that special handling of codes below
32 should be added, which I did. Using the parentheses is apparently an
existing practice elsewhere.
Note that neither DRM nor V4L2 currently has such fourcc codes currently.
>
> > + char *p = output;
> > + unsigned int i;
> > + u32 val;
> > +
> > + if (fmt[1] != 'c' || fmt[2] != 'c')
> > + return error_string(buf, end, "(%p4?)", spec);
> > +
> > + if (check_pointer(&buf, end, fourcc, spec))
> > + return buf;
> > +
> > + val = *fourcc & ~BIT(31);
> > +
> > + for (i = 0; i < sizeof(*fourcc); i++) {
> > + unsigned char c = val >> (i * 8);
>
> ...
>
> > + /* Weed out spaces */
> > + if (c == ' ')
> > + continue;
>
> None of the existing users does that. Why?
The existing instances of printing fourcc codes in V4L2 are scattered
around with priority on implementation simplicity. As we have a single
simplementation here, I'm not really worried about that.
>
> > + /* Print non-control ASCII characters as-is */
> > + if (isascii(c) && isprint(c)) {
> > + *p++ = c;
> > + continue;
> > + }
> > +
> > + *p++ = '(';
> > + p = hex_byte_pack(p, c);
> > + *p++ = ')';
> > + }
> > +
> > + strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
> > + p += strlen(p);
> > +
> > + *p++ = ' ';
> > + *p++ = '(';
>
> > + p = special_hex_number(p, output + sizeof(output) - 2, *fourcc,
> > + sizeof(u32));
>
> This is perfectly one line (in this file we have even longer lines).
Sure, you can do that, and I can then review your patch and point to the
coding style documentation. :-)
>
> > + *p++ = ')';
> > + *p = '\0';
> > +
> > + return string(buf, end, output, spec);
> > +}
>
--
Kind regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 3/3] drm/fourcc: Switch to %p4cc format modifier
Hi Daniel,
Thanks for the comments.
On Tue, Feb 09, 2021 at 08:27:10AM +0100, Daniel Vetter wrote:
> On Mon, Feb 8, 2021 at 9:20 PM Sakari Ailus
> wrote:
> >
> > Instead of constructing the FourCC code manually, use the %p4cc printk
> > modifier to print it. Also leave a message to avoid using this function.
> >
> > The next step would be to convert the users to use %p4cc directly instead
> > and removing the function.
> >
> > Signed-off-by: Sakari Ailus
> > ---
> > drivers/gpu/drm/drm_fourcc.c | 16 +++-
> > 1 file changed, 3 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> > index 03262472059c..4ff40f2f27c0 100644
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -30,11 +30,6 @@
> > #include
> > #include
> >
> > -static char printable_char(int c)
> > -{
> > - return isascii(c) && isprint(c) ? c : '?';
> > -}
> > -
> > /**
> > * drm_mode_legacy_fb_format - compute drm fourcc code from legacy
> > description
> > * @bpp: bits per pixels
> > @@ -134,17 +129,12 @@ EXPORT_SYMBOL(drm_driver_legacy_fb_format);
> > * drm_get_format_name - fill a string with a drm fourcc format's name
> > * @format: format to compute name of
> > * @buf: caller-supplied buffer
> > + *
> > + * Please use %p4cc printk format modifier instead of this function.
>
> I think would be nice if we could roll this out and outright delete
> this one here ... Quick git grep says there's not that many, and %p4cc
> is quite a bit shorter than what we have now.
Sounds good; I can submit patches for that but I think I'll do that once we
have the %p4cc modifier in.
--
Regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v6 1/3] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
pixel formats denoted by fourccs. The fourcc encoding is the same for both
so the same implementation can be used.
Suggested-by: Mauro Carvalho Chehab
Signed-off-by: Sakari Ailus
Reviewed-by: Petr Mladek
Reviewed-by: Sergey Senozhatsky
---
Documentation/core-api/printk-formats.rst | 16 +++
lib/test_printf.c | 17
lib/vsprintf.c| 51 +++
scripts/checkpatch.pl | 6 ++-
4 files changed, 88 insertions(+), 2 deletions(-)
diff --git a/Documentation/core-api/printk-formats.rst
b/Documentation/core-api/printk-formats.rst
index 160e710d992f..da2aa065dc42 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -567,6 +567,22 @@ For printing netdev_features_t.
Passed by reference.
+V4L2 and DRM FourCC code (pixel format)
+---
+
+::
+
+ %p4cc
+
+Print a FourCC code used by V4L2 or DRM, including format endianness and
+its numerical value as hexadecimal.
+
+Passed by reference.
+
+Examples::
+
+ %p4cc BG12 little-endian (0x32314742)
+
Thanks
==
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7d60f24240a4..78c94159d9d5 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -647,6 +647,22 @@ static void __init fwnode_pointer(void)
software_node_unregister_nodes(softnodes);
}
+static void __init fourcc_pointer(void)
+{
+ struct {
+ u32 code;
+ char *str;
+ } const try[] = {
+ { 0x20104646, "FF(10) little-endian (0x20104646)", },
+ { 0xa0104646, "FF(10) big-endian (0xa0104646)", },
+ { 0x10111213, "(13)(12)(11)(10) little-endian (0x10111213)", },
+ };
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(try); i++)
+ test(try[i].str, "%p4cc", &try[i].code);
+}
+
static void __init
errptr(void)
{
@@ -692,6 +708,7 @@ test_pointer(void)
flags();
errptr();
fwnode_pointer();
+ fourcc_pointer();
}
static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..ef50557e07b3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1733,6 +1733,54 @@ char *netdev_bits(char *buf, char *end, const void *addr,
return special_hex_number(buf, end, num, size);
}
+static noinline_for_stack
+char *fourcc_string(char *buf, char *end, const u32 *fourcc,
+ struct printf_spec spec, const char *fmt)
+{
+ char output[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")];
+ char *p = output;
+ unsigned int i;
+ u32 val;
+
+ if (fmt[1] != 'c' || fmt[2] != 'c')
+ return error_string(buf, end, "(%p4?)", spec);
+
+ if (check_pointer(&buf, end, fourcc, spec))
+ return buf;
+
+ val = *fourcc & ~BIT(31);
+
+ for (i = 0; i < sizeof(*fourcc); i++) {
+ unsigned char c = val >> (i * 8);
+
+ /* Weed out spaces */
+ if (c == ' ')
+ continue;
+
+ /* Print non-control ASCII characters as-is */
+ if (isascii(c) && isprint(c)) {
+ *p++ = c;
+ continue;
+ }
+
+ *p++ = '(';
+ p = hex_byte_pack(p, c);
+ *p++ = ')';
+ }
+
+ strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
+ p += strlen(p);
+
+ *p++ = ' ';
+ *p++ = '(';
+ p = special_hex_number(p, output + sizeof(output) - 2, *fourcc,
+ sizeof(u32));
+ *p++ = ')';
+ *p = '\0';
+
+ return string(buf, end, output, spec);
+}
+
static noinline_for_stack
char *address_val(char *buf, char *end, const void *addr,
struct printf_spec spec, const char *fmt)
@@ -2162,6 +2210,7 @@ char *fwnode_string(char *buf, char *end, struct
fwnode_handle *fwnode,
* correctness of the format string and va_list arguments.
* - 'K' For a kernel pointer that should be hidden from unprivileged users
* - 'NF' For a netdev_features_t
+ * - '4cc' V4L2 or DRM FourCC code, with endianness and raw numerical value.
* - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
*a certain separator (' ' by default):
* C colon
@@ -2259,6 +2308,8 @@ char *pointer(const char *fmt, char *buf, char *end, void
*ptr,
return restricted_pointer(buf, end, ptr, spec);
case 'N':
return netdev_bits(buf,
[PATCH v6 2/3] v4l: ioctl: Use %p4cc printk modifier to print FourCC codes
Now that we can print FourCC codes directly using printk, make use of the
feature in V4L2 core.
Signed-off-by: Sakari Ailus
---
drivers/media/v4l2-core/v4l2-ioctl.c | 85 +++-
1 file changed, 21 insertions(+), 64 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
b/drivers/media/v4l2-core/v4l2-ioctl.c
index 31d1342e61e8..31662c3a8c9e 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -265,13 +265,9 @@ static void v4l_print_fmtdesc(const void *arg, bool
write_only)
{
const struct v4l2_fmtdesc *p = arg;
- pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%c%c%c%c,
mbus_code=0x%04x, description='%.*s'\n",
+ pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%p4cc,
mbus_code=0x%04x, description='%.*s'\n",
p->index, prt_names(p->type, v4l2_type_names),
- p->flags, (p->pixelformat & 0xff),
- (p->pixelformat >> 8) & 0xff,
- (p->pixelformat >> 16) & 0xff,
- (p->pixelformat >> 24) & 0xff,
- p->mbus_code,
+ p->flags, &p->pixelformat, p->mbus_code,
(int)sizeof(p->description), p->description);
}
@@ -293,12 +289,8 @@ static void v4l_print_format(const void *arg, bool
write_only)
case V4L2_BUF_TYPE_VIDEO_CAPTURE:
case V4L2_BUF_TYPE_VIDEO_OUTPUT:
pix = &p->fmt.pix;
- pr_cont(", width=%u, height=%u, pixelformat=%c%c%c%c, field=%s,
bytesperline=%u, sizeimage=%u, colorspace=%d, flags=0x%x, ycbcr_enc=%u,
quantization=%u, xfer_func=%u\n",
- pix->width, pix->height,
- (pix->pixelformat & 0xff),
- (pix->pixelformat >> 8) & 0xff,
- (pix->pixelformat >> 16) & 0xff,
- (pix->pixelformat >> 24) & 0xff,
+ pr_cont(", width=%u, height=%u, pixelformat=%p4cc, field=%s,
bytesperline=%u, sizeimage=%u, colorspace=%d, flags=0x%x, ycbcr_enc=%u,
quantization=%u, xfer_func=%u\n",
+ pix->width, pix->height, &pix->pixelformat,
prt_names(pix->field, v4l2_field_names),
pix->bytesperline, pix->sizeimage,
pix->colorspace, pix->flags, pix->ycbcr_enc,
@@ -307,12 +299,8 @@ static void v4l_print_format(const void *arg, bool
write_only)
case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
mp = &p->fmt.pix_mp;
- pr_cont(", width=%u, height=%u, format=%c%c%c%c, field=%s,
colorspace=%d, num_planes=%u, flags=0x%x, ycbcr_enc=%u, quantization=%u,
xfer_func=%u\n",
- mp->width, mp->height,
- (mp->pixelformat & 0xff),
- (mp->pixelformat >> 8) & 0xff,
- (mp->pixelformat >> 16) & 0xff,
- (mp->pixelformat >> 24) & 0xff,
+ pr_cont(", width=%u, height=%u, format=%p4cc, field=%s,
colorspace=%d, num_planes=%u, flags=0x%x, ycbcr_enc=%u, quantization=%u,
xfer_func=%u\n",
+ mp->width, mp->height, &mp->pixelformat,
prt_names(mp->field, v4l2_field_names),
mp->colorspace, mp->num_planes, mp->flags,
mp->ycbcr_enc, mp->quantization, mp->xfer_func);
@@ -337,13 +325,9 @@ static void v4l_print_format(const void *arg, bool
write_only)
case V4L2_BUF_TYPE_VBI_CAPTURE:
case V4L2_BUF_TYPE_VBI_OUTPUT:
vbi = &p->fmt.vbi;
- pr_cont(", sampling_rate=%u, offset=%u, samples_per_line=%u,
sample_format=%c%c%c%c, start=%u,%u, count=%u,%u\n",
+ pr_cont(", sampling_rate=%u, offset=%u, samples_per_line=%u,
sample_format=%p4cc, start=%u,%u, count=%u,%u\n",
vbi->sampling_rate, vbi->offset,
- vbi->samples_per_line,
- (vbi->sample_format & 0xff),
- (vbi->sample_format >> 8) & 0xff,
- (vbi->sample_format >> 16) & 0xff,
- (vbi->sample_format >> 24) & 0xff,
+ vbi->samples_per_line, &vbi->sample_format,
vbi->start[0], vbi->start[1],
vbi->count[0], vbi->count[1]);
break;
@@ -360,21 +344,13 @@ static void v4l_print_format(const void *arg, bool
write_only)
case V4L2_BUF_TYPE_SDR_CAPTURE:
[PATCH v6 0/3] Add %p4cc printk modifier for V4L2 and DRM fourcc codes
Hi all, This set adds support for %p4cc printk modifier for printing V4L2 and DRM fourcc codes. The codes are cumbersome to print manually and by adding the modifier, this task is saved from the V4L2 and DRM frameworks as well as related drivers. DRM actually had it handled in a way (see 3rd patch) but the printk modifier makes printing the format easier even there. On V4L2 side it saves quite a few lines of repeating different implementations of printing the 4cc codes. Further work will include converting the V4L2 drivers doing the same, as well as converting DRM drivers from drm_get_format_name() to plain %p4cc. I left these out from this version since individual drivers are easier changed without dealing with multiple trees. If DRM folks would prefer to convert drivers to %p4cc directly instead I have no problem dropping the 3rd patch. Nearly all uses in DRM are in printk family of functions that can readily use %p4cc instead of the current arrangement that relies on caller-allocated temporary buffer. Since v5: - Added V4L2 core conversion to %p4cc, as well as change the DRM fourcc printing function to use %p4cc. - Add missing checkpatch.pl checks for %p4cc modifier. Sakari Ailus (3): lib/vsprintf: Add support for printing V4L2 and DRM fourccs v4l: ioctl: Use %p4cc printk modifier to print FourCC codes drm/fourcc: Switch to %p4cc format modifier Documentation/core-api/printk-formats.rst | 16 + drivers/gpu/drm/drm_fourcc.c | 16 + drivers/media/v4l2-core/v4l2-ioctl.c | 85 ++- lib/test_printf.c | 17 + lib/vsprintf.c| 51 ++ scripts/checkpatch.pl | 6 +- 6 files changed, 112 insertions(+), 79 deletions(-) -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v6 3/3] drm/fourcc: Switch to %p4cc format modifier
Instead of constructing the FourCC code manually, use the %p4cc printk
modifier to print it. Also leave a message to avoid using this function.
The next step would be to convert the users to use %p4cc directly instead
and removing the function.
Signed-off-by: Sakari Ailus
---
drivers/gpu/drm/drm_fourcc.c | 16 +++-
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 03262472059c..4ff40f2f27c0 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -30,11 +30,6 @@
#include
#include
-static char printable_char(int c)
-{
- return isascii(c) && isprint(c) ? c : '?';
-}
-
/**
* drm_mode_legacy_fb_format - compute drm fourcc code from legacy description
* @bpp: bits per pixels
@@ -134,17 +129,12 @@ EXPORT_SYMBOL(drm_driver_legacy_fb_format);
* drm_get_format_name - fill a string with a drm fourcc format's name
* @format: format to compute name of
* @buf: caller-supplied buffer
+ *
+ * Please use %p4cc printk format modifier instead of this function.
*/
const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf
*buf)
{
- snprintf(buf->str, sizeof(buf->str),
-"%c%c%c%c %s-endian (0x%08x)",
-printable_char(format & 0xff),
-printable_char((format >> 8) & 0xff),
-printable_char((format >> 16) & 0xff),
-printable_char((format >> 24) & 0x7f),
-format & DRM_FORMAT_BIG_ENDIAN ? "big" : "little",
-format);
+ snprintf(buf->str, sizeof(buf->str), "%p4cc", &format);
return buf->str;
}
--
2.29.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] dt-bindings: Drop unnecessary *-supply schemas properties
Hi Rob, On Mon, Dec 21, 2020 at 04:46:59PM -0700, Rob Herring wrote: > *-supply properties are always a single phandle, so binding schemas > don't need a type $ref nor 'maxItems'. > > A meta-schema check for this is pending once these existing cases are > fixed. > > Cc: Jonathan Cameron > Cc: Dmitry Torokhov > Cc: Laurent Pinchart > Cc: Mauro Carvalho Chehab > Cc: Sakari Ailus > Cc: Lee Jones > Cc: Mark Brown > Cc: Maxime Ripard > Cc: dri-devel@lists.freedesktop.org > Cc: linux-...@vger.kernel.org > Cc: linux-in...@vger.kernel.org > Cc: linux-me...@vger.kernel.org > Signed-off-by: Rob Herring On media bits: Acked-by: Sakari Ailus -- Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Andy, On Thu, Dec 10, 2020 at 03:05:02PM +0200, Andy Shevchenko wrote: > On Thu, Dec 10, 2020 at 2:16 PM Petr Mladek wrote: > > On Fri 2020-11-13 12:54:41, Sakari Ailus wrote: > > > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM > > > pixel formats denoted by fourccs. The fourcc encoding is the same for both > > > so the same implementation can be used. > > > > > > Suggested-by: Mauro Carvalho Chehab > > > Signed-off-by: Sakari Ailus > > > > Andy, Rasmus, > > > > the last version looks fine to me. I am going to push it. > > Please, speak up if you are against it. > > My concerns are: > - not so standard format of representation (why not to use > string_escape_mem() helper?) or is it? The format string may contain spaces that are not meant to be printed. Other unprintable chacaters should not be present (at least not in V4L2 pixelformats). The hexadecimal representation is there to convey the numerical value and that originally came from DRM, not V4L2. > - no compatibility with generic 4cc > (I would rather have an additional specifier here for v4l2 cases. What do you mean by "generic 4cc"? There are two users of 4cc codes in the kernel that I know of: V4L2 and DRM. Something that does not refer to in-memory pixel formats? > OTOH generic %p4cc to me sounds like an equivalent to %4pEh (but we > have similar cases with MAC where %6ph is the same as %pM). > > But I'm not insisting on them, consider it like just my 2 cents to the > discussion. Ack. -- Kind regards, Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v5 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
pixel formats denoted by fourccs. The fourcc encoding is the same for both
so the same implementation can be used.
Suggested-by: Mauro Carvalho Chehab
Signed-off-by: Sakari Ailus
---
since v4:
- Use correct destination for error string (was broken in v4)
- Remove unneeded comment "/* subtract parenthesis and the space from the
size */".
Documentation/core-api/printk-formats.rst | 16 +++
lib/test_printf.c | 17
lib/vsprintf.c| 51 +++
3 files changed, 84 insertions(+)
diff --git a/Documentation/core-api/printk-formats.rst
b/Documentation/core-api/printk-formats.rst
index 6d26c5c6ac48..080262d2e030 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -565,6 +565,22 @@ For printing netdev_features_t.
Passed by reference.
+V4L2 and DRM FourCC code (pixel format)
+---
+
+::
+
+ %p4cc
+
+Print a FourCC code used by V4L2 or DRM, including format endianness and
+its numerical value as hexadecimal.
+
+Passed by reference.
+
+Examples::
+
+ %p4cc BG12 little-endian (0x32314742)
+
Thanks
==
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7ac87f18a10f..7fb042542660 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -649,6 +649,22 @@ static void __init fwnode_pointer(void)
software_node_unregister(&softnodes[0]);
}
+static void __init fourcc_pointer(void)
+{
+ struct {
+ u32 code;
+ char *str;
+ } const try[] = {
+ { 0x20104646, "FF(10) little-endian (0x20104646)", },
+ { 0xa0104646, "FF(10) big-endian (0xa0104646)", },
+ { 0x10111213, "(13)(12)(11)(10) little-endian (0x10111213)", },
+ };
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(try); i++)
+ test(try[i].str, "%p4cc", &try[i].code);
+}
+
static void __init
errptr(void)
{
@@ -694,6 +710,7 @@ test_pointer(void)
flags();
errptr();
fwnode_pointer();
+ fourcc_pointer();
}
static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 14c9a6af1b23..b07ee5b7de06 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1733,6 +1733,54 @@ char *netdev_bits(char *buf, char *end, const void *addr,
return special_hex_number(buf, end, num, size);
}
+static noinline_for_stack
+char *fourcc_string(char *buf, char *end, const u32 *fourcc,
+ struct printf_spec spec, const char *fmt)
+{
+ char output[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")];
+ char *p = output;
+ unsigned int i;
+ u32 val;
+
+ if (fmt[1] != 'c' || fmt[2] != 'c')
+ return error_string(buf, end, "(%p4?)", spec);
+
+ if (check_pointer(&buf, end, fourcc, spec))
+ return buf;
+
+ val = *fourcc & ~BIT(31);
+
+ for (i = 0; i < sizeof(*fourcc); i++) {
+ unsigned char c = val >> (i * 8);
+
+ /* Weed out spaces */
+ if (c == ' ')
+ continue;
+
+ /* Print non-control ASCII characters as-is */
+ if (isascii(c) && isprint(c)) {
+ *p++ = c;
+ continue;
+ }
+
+ *p++ = '(';
+ p = hex_byte_pack(p, c);
+ *p++ = ')';
+ }
+
+ strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
+ p += strlen(p);
+
+ *p++ = ' ';
+ *p++ = '(';
+ p = special_hex_number(p, output + sizeof(output) - 2, *fourcc,
+ sizeof(u32));
+ *p++ = ')';
+ *p = '\0';
+
+ return string(buf, end, output, spec);
+}
+
static noinline_for_stack
char *address_val(char *buf, char *end, const void *addr,
struct printf_spec spec, const char *fmt)
@@ -2162,6 +2210,7 @@ char *fwnode_string(char *buf, char *end, struct
fwnode_handle *fwnode,
* correctness of the format string and va_list arguments.
* - 'K' For a kernel pointer that should be hidden from unprivileged users
* - 'NF' For a netdev_features_t
+ * - '4cc' V4L2 or DRM FourCC code, with endianness and raw numerical value.
* - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
*a certain separator (' ' by default):
* C colon
@@ -2259,6 +2308,8 @@ char *pointer(const char *fmt, char *buf, char *end, void
*ptr,
return restricted_pointer(buf, end, ptr, spec);
Re: [PATCH v4 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Joe,
On Tue, Nov 03, 2020 at 08:49:36AM -0800, Joe Perches wrote:
> On Tue, 2020-11-03 at 16:56 +0200, Sakari Ailus wrote:
> > On Tue, Nov 03, 2020 at 04:47:47PM +0200, Andy Shevchenko wrote:
> > > On Tue, Nov 03, 2020 at 03:34:00PM +0200, Sakari Ailus wrote:
> > > > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
> > > > pixel formats denoted by fourccs. The fourcc encoding is the same for
> > > > both
> > > > so the same implementation can be used.
> > >
> > > ...
> > >
> > > > +static noinline_for_stack
> > > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > > + struct printf_spec spec, const char *fmt)
> > > > +{
> > > > + char output[sizeof("(xx)(xx)(xx)(xx) little-endian
> > > > (0x01234567)")];
> > >
> > > I would add a comment that there is another possibility, i.e. big-endian,
> > > but
> > > it occupies less space.
>
> I think it's unnecessary as it's obvious and similarly done in other
> _string type functions.
>
> > > > + p = special_hex_number(p, output + sizeof(output) - 2, *fourcc,
> > > > + sizeof(u32));
> > >
> > > I would go with one line here.
> >
> > It's wrapped since the result would be over 80 otherwise.
>
> Perhaps simpler as
>
> p = special_hex_number(p, p + 10, *fourcc, sizeof(u32));
Yes. But having bugs elsewhere would have a magnified effect. I wouldn't be
afraid of a newline here.
>
> > > The (theoretical) problem is here that the case when buffer size is not
> > > enough
> > > to print a value will be like '(0xabc)' but should be rather '(0xabcd'
> > > like
> > > snprintf() does in general.
>
> Isn't the stack buffer known to be large enough?
Yes. If there are no bugs, that is.
>
> > > > + *p++ = ')';
> > > > + *p = '\0';
> > > > +
> > > > + return string(buf, end, output, spec);
>
> Isn't the actual output buffer used here truncating output?
>
> If the general problem is someone using a limited length pointer
> output like %10p4cc, then all the output is getting truncated no?
Correct. But I'd also be surprised if anyone ever used this feature.
--
Kind regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Andy,
On Tue, Nov 03, 2020 at 04:47:47PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 03, 2020 at 03:34:00PM +0200, Sakari Ailus wrote:
> > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
> > pixel formats denoted by fourccs. The fourcc encoding is the same for both
> > so the same implementation can be used.
>
> ...
>
> > +static noinline_for_stack
> > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > + struct printf_spec spec, const char *fmt)
> > +{
> > + char output[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")];
>
> I would add a comment that there is another possibility, i.e. big-endian, but
> it occupies less space.
This string is here to represent the longest possible output of the
function. Most of the time it's less. Saying that might make sense but it's
fairly clear already. Either way works for me though.
>
> > + char *p = output;
> > + unsigned int i;
> > + u32 val;
> > +
> > + if (fmt[1] != 'c' || fmt[2] != 'c')
> > + return error_string(output, end, "(%p4?)", spec);
> > +
> > + if (check_pointer(&buf, end, fourcc, spec))
> > + return buf;
> > +
> > + val = *fourcc & ~BIT(31);
> > +
> > + for (i = 0; i < sizeof(*fourcc); i++) {
> > + unsigned char c = val >> (i * 8);
> > +
> > + /* Weed out spaces */
> > + if (c == ' ')
> > + continue;
> > +
> > + /* Print non-control ASCII characters as-is */
> > + if (isascii(c) && isprint(c)) {
> > + *p++ = c;
> > + continue;
> > + }
> > +
> > + *p++ = '(';
> > + p = hex_byte_pack(p, c);
> > + *p++ = ')';
> > + }
>
> I guess above loop can be still optimized, but I have to think about it.
>
> > + strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
> > + p += strlen(p);
> > +
> > + *p++ = ' ';
> > + *p++ = '(';
> > + /* subtract parenthesis and the space from the size */
>
> This comment misleading. What you are subtracting is a room for closing
> parenthesis and NUL.
Agreed, I'll update it for v5.
>
> > + p = special_hex_number(p, output + sizeof(output) - 2, *fourcc,
> > + sizeof(u32));
>
> I would go with one line here.
It's wrapped since the result would be over 80 otherwise.
>
> The (theoretical) problem is here that the case when buffer size is not enough
> to print a value will be like '(0xabc)' but should be rather '(0xabcd' like
> snprintf() does in general.
>
> > + *p++ = ')';
> > + *p = '\0';
> > +
> > + return string(buf, end, output, spec);
> > +}
>
--
Regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
pixel formats denoted by fourccs. The fourcc encoding is the same for both
so the same implementation can be used.
Suggested-by: Mauro Carvalho Chehab
Signed-off-by: Sakari Ailus
---
Hi folks,
I believe I've addressed comments from Rasmus, Joe and Andy --- variables
that don't need to be negative have remained unsigned though. Thanks for
the suggestions, I believe this is much cleaner than v3.
since v3:
- Remove use of WARN_ON, assume the code is correct instead. A side effect
of this is the code is much easier to understand.
- Check the modifier first before validating the buf pointer.
- Use isascii() and isprint() functions to weed out non-printable
characters.
- Use hex_byte_pack() for printing 8-bit hexadecimal numbers.
- Remove macros, and instead use plain strings.
- Use strcpy() to copy the endianness string, and then strlen() to add its
length.
- Strip the MSB (endianness bit), then use the value as such.
- Assign characters to the buffer and increment active pointer at the same
time.
- Drop __ from variable names.
- Add example to documentation.
Documentation/core-api/printk-formats.rst | 16 +++
lib/test_printf.c | 17
lib/vsprintf.c| 52 +++
3 files changed, 85 insertions(+)
diff --git a/Documentation/core-api/printk-formats.rst
b/Documentation/core-api/printk-formats.rst
index 6d26c5c6ac48..080262d2e030 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -565,6 +565,22 @@ For printing netdev_features_t.
Passed by reference.
+V4L2 and DRM FourCC code (pixel format)
+---
+
+::
+
+ %p4cc
+
+Print a FourCC code used by V4L2 or DRM, including format endianness and
+its numerical value as hexadecimal.
+
+Passed by reference.
+
+Examples::
+
+ %p4cc BG12 little-endian (0x32314742)
+
Thanks
==
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7ac87f18a10f..7fb042542660 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -649,6 +649,22 @@ static void __init fwnode_pointer(void)
software_node_unregister(&softnodes[0]);
}
+static void __init fourcc_pointer(void)
+{
+ struct {
+ u32 code;
+ char *str;
+ } const try[] = {
+ { 0x20104646, "FF(10) little-endian (0x20104646)", },
+ { 0xa0104646, "FF(10) big-endian (0xa0104646)", },
+ { 0x10111213, "(13)(12)(11)(10) little-endian (0x10111213)", },
+ };
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(try); i++)
+ test(try[i].str, "%p4cc", &try[i].code);
+}
+
static void __init
errptr(void)
{
@@ -694,6 +710,7 @@ test_pointer(void)
flags();
errptr();
fwnode_pointer();
+ fourcc_pointer();
}
static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 14c9a6af1b23..2be86b302c88 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1733,6 +1733,55 @@ char *netdev_bits(char *buf, char *end, const void *addr,
return special_hex_number(buf, end, num, size);
}
+static noinline_for_stack
+char *fourcc_string(char *buf, char *end, const u32 *fourcc,
+ struct printf_spec spec, const char *fmt)
+{
+ char output[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")];
+ char *p = output;
+ unsigned int i;
+ u32 val;
+
+ if (fmt[1] != 'c' || fmt[2] != 'c')
+ return error_string(output, end, "(%p4?)", spec);
+
+ if (check_pointer(&buf, end, fourcc, spec))
+ return buf;
+
+ val = *fourcc & ~BIT(31);
+
+ for (i = 0; i < sizeof(*fourcc); i++) {
+ unsigned char c = val >> (i * 8);
+
+ /* Weed out spaces */
+ if (c == ' ')
+ continue;
+
+ /* Print non-control ASCII characters as-is */
+ if (isascii(c) && isprint(c)) {
+ *p++ = c;
+ continue;
+ }
+
+ *p++ = '(';
+ p = hex_byte_pack(p, c);
+ *p++ = ')';
+ }
+
+ strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
+ p += strlen(p);
+
+ *p++ = ' ';
+ *p++ = '(';
+ /* subtract parenthesis and the space from the size */
+ p = special_hex_number(p, output + sizeof(output) - 2, *fourcc,
+ sizeof(u32));
+ *p++ = ')';
+ *p = '\0';
+
+ return string(buf, end, output, spec);
+}
+
static noinline_for_stack
char *address_val(char *buf, char *end,
Re: [RESEND PATCH v3 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Rasmus,
Thanks for the review.
On Mon, Apr 27, 2020 at 05:44:00PM +0200, Rasmus Villemoes wrote:
> On 27/04/2020 16.53, Sakari Ailus wrote:
> > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
> > pixel formats denoted by fourccs. The fourcc encoding is the same for both
> > so the same implementation can be used.
> >
> > Suggested-by: Mauro Carvalho Chehab
> > Signed-off-by: Sakari Ailus
> > ---
> > since v2:
> >
> > - Add comments to explain why things are being done
> >
> > - Print characters under 32 (space) as hexadecimals in parenthesis.
> >
> > - Do not print spaces in the fourcc codes.
> >
> > - Make use of a loop over the fourcc characters instead of
> > put_unaligned_le32(). This is necessary to omit spaces in the output.
> >
> > - Use DRM style format instead of V4L2. This provides the precise code as
> > a numerical value as well as explicit endianness information.
> >
> > - Added WARN_ON_ONCE() sanity checks. Comments on these are welcome; I'd
> > expect them mostly be covered by the tests.
> >
> > - Added tests for %p4cc in lib/test_printf.c
> >
> > Documentation/core-api/printk-formats.rst | 12
> > lib/test_printf.c | 17 +
> > lib/vsprintf.c| 86 +++
> > 3 files changed, 115 insertions(+)
> >
> > diff --git a/Documentation/core-api/printk-formats.rst
> > b/Documentation/core-api/printk-formats.rst
> > index 8ebe46b1af39..7aa0451e06fb 100644
> > --- a/Documentation/core-api/printk-formats.rst
> > +++ b/Documentation/core-api/printk-formats.rst
> > @@ -545,6 +545,18 @@ For printing netdev_features_t.
> >
> > Passed by reference.
> >
> > +V4L2 and DRM FourCC code (pixel format)
> > +---
> > +
> > +::
> > +
> > + %p4cc
> > +
> > +Print a FourCC code used by V4L2 or DRM, including format endianness and
> > +its numerical value as hexadecimal.
> > +
> > +Passed by reference.
> > +
> > Thanks
> > ==
> >
> > diff --git a/lib/test_printf.c b/lib/test_printf.c
> > index 2d9f520d2f27..a14754086707 100644
> > --- a/lib/test_printf.c
> > +++ b/lib/test_printf.c
> > @@ -624,6 +624,22 @@ static void __init fwnode_pointer(void)
> > software_node_unregister_nodes(softnodes);
> > }
> >
> > +static void __init fourcc_pointer(void)
> > +{
> > + struct {
> > + u32 code;
> > + char *str;
> > + } const try[] = {
> > + { 0x20104646, "FF(10) little-endian (0x20104646)", },
> > + { 0xa0104646, "FF(10) big-endian (0xa0104646)", },
> > + { 0x10111213, "(13)(12)(11)(10) little-endian (0x10111213)", },
> > + };
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(try); i++)
> > + test(try[i].str, "%p4cc", &try[i].code);
> > +}
> > +
> > static void __init
> > errptr(void)
> > {
> > @@ -668,6 +684,7 @@ test_pointer(void)
> > flags();
> > errptr();
> > fwnode_pointer();
> > + fourcc_pointer();
> > }
> >
> > static void __init selftest(void)
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 7c488a1ce318..02e7906619c0 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1721,6 +1721,89 @@ char *netdev_bits(char *buf, char *end, const void
> > *addr,
> > return special_hex_number(buf, end, num, size);
> > }
> >
> > +static noinline_for_stack
> > +char *fourcc_string(char *buf, char *end, const u32 *__fourcc,
> > + struct printf_spec spec, const char *fmt)
> > +{
> > +#define FOURCC_HEX_CHAR_STR"(xx)"
> > +#define FOURCC_BIG_ENDIAN_STR " big-endian"
> > +#define FOURCC_LITTLE_ENDIAN_STR " little-endian"
> > +#define FOURCC_HEX_NUMBER " (0x01234567)"
> > +#define FOURCC_STRING_MAX \
> > + FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR \
> > + FOURCC_HEX_CHAR_STR FOURCC_LITTLE_ENDIAN_STR FOURCC_HEX_NUMBER
> > + struct printf_spec my_spec = {
> > + .type = FORMAT_TYPE_UINT,
> > + .field_width = 2,
> > + .flags = SMALL,
> > + .base = 16,
> > + .precision = -1,
> > +
Re: [PATCH 17/18] media/omap3isp: Clean up IOMMU workaround
On Thu, Aug 20, 2020 at 06:25:19PM +0100, Robin Murphy wrote: > On 2020-08-20 17:53, Sakari Ailus wrote: > > Hi Robin, > > > > On Thu, Aug 20, 2020 at 04:08:36PM +0100, Robin Murphy wrote: > > > Now that arch/arm is wired up for default domains and iommu-dma, devices > > > behind IOMMUs will get mappings set up automatically as appropriate, so > > > there is no need for drivers to do so manually. > > > > > > Signed-off-by: Robin Murphy > > > > Thanks for the patch. > > Many thanks for testing so quickly! > > > I haven't looked at the details but it seems that this causes the buffer > > memory allocation to be physically contiguous, which causes a failure to > > allocate video buffers of entirely normal size. I guess that was not > > intentional? > > Hmm, it looks like the device ends up with the wrong DMA ops, which implies > something didn't go as expected with the earlier IOMMU setup and default > domain creation. Chances are that either I missed some subtlety in the > omap_iommu change, or I've fundamentally misjudged how the ISP probing works > and it never actually goes down the of_iommu_configure() path in the first > place. Do you get any messages from the IOMMU layer earlier on during boot? I do get these: [2.934936] iommu: Default domain type: Translated [2.940917] omap-iommu 480bd400.mmu: 480bd400.mmu registered [2.946899] platform 480bc000.isp: Adding to iommu group 0 -- Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 17/18] media/omap3isp: Clean up IOMMU workaround
09a0 ef30 010eb400 [ 219.35] ffe0: 0001716c bee65104 b588 b6e413ac [ 219.394409] r10:0036 r9:e6d7e000 r8:c0100244 r7:0036 r6: r5:bee65c1a [ 219.402740] r4: [ 219.405426] irq event stamp: 5075 [ 219.408905] hardirqs last enabled at (5083): [] console_unlock+0x4cc/0x524 [ 219.417297] hardirqs last disabled at (5092): [] console_unlock+0x3c8/0x524 [ 219.425628] softirqs last enabled at (4532): [] __do_softirq+0x1f0/0x490 [ 219.433837] softirqs last disabled at (4493): [] irq_exit+0xe4/0x160 [ 219.441558] ---[ end trace 8c56810633cf24db ]--- [ 219.446502] omap3isp 480bc000.isp: dma_alloc_coherent of size 17743872 failed -8<--- -- Kind regards, Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/fourcc: Add bayer formats and modifiers
- - - - - + - - - - - - - - + - - - - - > - - - + - - - - - - - - + > + *| r1 r1 g1 g1 r0 r0 g0 g0 | g2 g2 g2 g2 g2 g2 g2 g2 | r2 r2 r2 r2 r2 > r2 r2 r2 | g3 g3 g3 g3 g3 g3 g3 g3 | > + *+ - - - - - - - - + - - - - - - - - + - - - - - > - - - + - - - - - - - - + > + *| r3 r3 r3 r3 r3 r3 r3 r3 | r3 r3 g3 g3 r2 r2 g2 g2 | > + *+ - - - - - - - - + - - - - - - - - + > + * > + * - 12-bits per sample > + * Every two consecutive samples are packed into three bytes. Each of the > + * first two bytes contain the 8 high order bits of the pixels, and the > third > + * byte contains the four least-significant bits of each pixel, in the same > + * order. > + * > + * For example, DRM_FORMAT_BAYER_GRBG12 with MIPI_FORMAT_MOD_CSI2_PACKED: > + * > + *0 row 0 (GRGR) > 31 > + *+ - - - - - - - - + - - - - - - - - + - - - - - > - - - + - - - - - - - - + > + *| g0 g0 g0 g0 g0 g0 g0 g0 | r0 r0 r0 r0 r0 r0 r0 r0 | r0 r0 r0 r0 g0 > g0 g0 g0 | g1 g1 g1 g1 g1 g1 g1 g1 | > + *+ - - - - - - - - + - - - - - - - - + - - - - - > - - - + - - - - - - - - + > + *| r1 r1 r1 r1 r1 r1 r1 r1 | r1 r1 r1 r1 g1 g1 g1 g1 | > + *+ - - - - - - - - + - - - - - - - - + > + * > + *0 row 1 (BGBG) > 31 > + *+ - - - - - - - - + - - - - - - - - + - - - - - > - - - + - - - - - - - - + > + *| b0 b0 b0 b0 b0 b0 b0 b0 | g0 g0 g0 g0 g0 g0 g0 g0 | g0 g0 g0 g0 b0 > b0 b0 b0 | b1 b1 b1 b1 b1 b1 b1 b1 | > + *+ - - - - - - - - + - - - - - - - - + - - - - - > - - - + - - - - - - - - + > + *| g1 g1 g1 g1 g1 g1 g1 g1 | g1 g1 g1 g1 b1 b1 b1 b1 | > + *+ - - - - - - - - + - - - - - - - - + > + * > + * - 14-bits per sample > + * Every four consecutive samples are packed into seven bytes. Each of the > + * first four bytes contain the eight high order bits of the pixels, and > the > + * three following bytes contains the six least-significant bits of each > + * pixel, in the same order. > + * > + * For example, DRM_FORMAT_BAYER_GBRG14 with MIPI_FORMAT_MOD_CSI2_PACKED: > + * > + *0row 0 (GBGBGBGB) > 31 > + *+ - - - - - - - - + - - - - - - - - + - - - - - > - - - + - - - - - - - - + > + *| g0 g0 g0 g0 g0 g0 g0 g0 | b0 b0 b0 b0 b0 b0 b0 b0 | g1 g1 g1 g1 g1 > g1 g1 g1 | b1 b1 b1 b1 b1 b1 b1 b1 | > + *+ - - - - - - - - + - - - - - - - - + - - - - - > - - - + - - - - - - - - + > + *| b0 b0 g0 g0 g0 g0 g0 g0 | g1 g1 g1 g1 b0 b0 b0 b0 | b1 b1 b1 b1 b1 > b1 g1 g1 | g2 g2 g2 g2 g2 g2 g2 g2 | > + *+ - - - - - - - - + - - - - - - - - + - - - - - > - - - + - - - - - - - - + > + *| b2 b2 b2 b2 b2 b2 b2 b2 | g3 g3 g3 g3 g3 g3 g3 g3 | b3 b3 b3 b3 b3 > b3 b3 b3 | b2 b2 g2 g2 g2 g2 g2 g2 | > + *+ - - - - - - - - + - - - - - - - - + - - - - - > - - - + - - - - - - - - + > + *| g3 g3 g3 g3 b2 b2 b2 b2 | b3 b3 b3 b3 b3 b3 g3 g3 | > + *+ - - - - - - - - + - - - - - - - - + > + * > + *0row 1 (RGRGRGRG) > 31 > + *+ - - - - - - - - + - - - - - - - - + - - - - - > - - - + - - - - - - - - + > + *| r0 r0 r0 r0 r0 r0 r0 r0 | g0 g0 g0 g0 g0 g0 g0 g0 | r1 r1 r1 r1 r1 > r1 r1 r1 | g1 g1 g1 g1 g1 g1 g1 g1 | > + *+ - - - - - - - - + - - - - - - - - + - - - - - > - - - + - - - - - - - - + > + *| g0 g0 r0 r0 r0 r0 r0 r0 | r1 r1 r1 r1 g0 g0 g0 g0 | g1 g1 g1 g1 g1 > g1 r1 r1 | r2 r2 r2 r2 r2 r2 r2 r2 | > + *+ - - - - - - - - + - - - - - - - - + - - - - - > - - - + - - - - - - - - + > + *| g2 g2 g2 g2 g2 g2 g2 g2 | r3 r3 r3 r3 r3 r3 r3 r3 | g3 g3 g3 g3 g3 > g3 g3 g3 | g2 g2 r2 r2 r2 r2 r2 r2 | > + *+ - - - - - - - - + - - - - - - - - + - - - - - > - - - + - - - - - - - - + > + *| r3 r3 r3 r3 g2 g2 g2 g2 | g3 g3 g3 g3 g3 g3 r3 r3 | > + *+ - - - - - - - - + - - - - - - - - + > + */ > +#define MIPI_FORMAT_MOD_CSI2_PACKED fourcc_mod_code(MIPI, 1) > + > #if defined(__cplusplus) > } > #endif -- Kind regards, Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
On Mon, Apr 27, 2020 at 05:50:07PM +0300, Sakari Ailus wrote: > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM > pixel formats denoted by fourccs. The fourcc encoding is the same for both > so the same implementation can be used. > > Suggested-by: Mauro Carvalho Chehab > Signed-off-by: Sakari Ailus Please ignore this one. -- Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RESEND PATCH v3 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
pixel formats denoted by fourccs. The fourcc encoding is the same for both
so the same implementation can be used.
Suggested-by: Mauro Carvalho Chehab
Signed-off-by: Sakari Ailus
---
since v2:
- Add comments to explain why things are being done
- Print characters under 32 (space) as hexadecimals in parenthesis.
- Do not print spaces in the fourcc codes.
- Make use of a loop over the fourcc characters instead of
put_unaligned_le32(). This is necessary to omit spaces in the output.
- Use DRM style format instead of V4L2. This provides the precise code as
a numerical value as well as explicit endianness information.
- Added WARN_ON_ONCE() sanity checks. Comments on these are welcome; I'd
expect them mostly be covered by the tests.
- Added tests for %p4cc in lib/test_printf.c
Documentation/core-api/printk-formats.rst | 12
lib/test_printf.c | 17 +
lib/vsprintf.c| 86 +++
3 files changed, 115 insertions(+)
diff --git a/Documentation/core-api/printk-formats.rst
b/Documentation/core-api/printk-formats.rst
index 8ebe46b1af39..7aa0451e06fb 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -545,6 +545,18 @@ For printing netdev_features_t.
Passed by reference.
+V4L2 and DRM FourCC code (pixel format)
+---
+
+::
+
+ %p4cc
+
+Print a FourCC code used by V4L2 or DRM, including format endianness and
+its numerical value as hexadecimal.
+
+Passed by reference.
+
Thanks
==
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 2d9f520d2f27..a14754086707 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -624,6 +624,22 @@ static void __init fwnode_pointer(void)
software_node_unregister_nodes(softnodes);
}
+static void __init fourcc_pointer(void)
+{
+ struct {
+ u32 code;
+ char *str;
+ } const try[] = {
+ { 0x20104646, "FF(10) little-endian (0x20104646)", },
+ { 0xa0104646, "FF(10) big-endian (0xa0104646)", },
+ { 0x10111213, "(13)(12)(11)(10) little-endian (0x10111213)", },
+ };
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(try); i++)
+ test(try[i].str, "%p4cc", &try[i].code);
+}
+
static void __init
errptr(void)
{
@@ -668,6 +684,7 @@ test_pointer(void)
flags();
errptr();
fwnode_pointer();
+ fourcc_pointer();
}
static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7c488a1ce318..02e7906619c0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1721,6 +1721,89 @@ char *netdev_bits(char *buf, char *end, const void *addr,
return special_hex_number(buf, end, num, size);
}
+static noinline_for_stack
+char *fourcc_string(char *buf, char *end, const u32 *__fourcc,
+ struct printf_spec spec, const char *fmt)
+{
+#define FOURCC_HEX_CHAR_STR"(xx)"
+#define FOURCC_BIG_ENDIAN_STR " big-endian"
+#define FOURCC_LITTLE_ENDIAN_STR " little-endian"
+#define FOURCC_HEX_NUMBER " (0x01234567)"
+#define FOURCC_STRING_MAX \
+ FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR \
+ FOURCC_HEX_CHAR_STR FOURCC_LITTLE_ENDIAN_STR FOURCC_HEX_NUMBER
+ struct printf_spec my_spec = {
+ .type = FORMAT_TYPE_UINT,
+ .field_width = 2,
+ .flags = SMALL,
+ .base = 16,
+ .precision = -1,
+ };
+ char __s[sizeof(FOURCC_STRING_MAX)];
+ char *s = __s;
+ unsigned int i;
+ /*
+* The 31st bit defines the endianness of the data, so save its printing
+* for later.
+*/
+ u32 fourcc = *__fourcc & ~BIT(31);
+ int ret;
+
+ if (check_pointer(&buf, end, __fourcc, spec))
+ return buf;
+
+ if (fmt[1] != 'c' || fmt[2] != 'c')
+ return error_string(buf, end, "(%p4?)", spec);
+
+ for (i = 0; i < sizeof(fourcc); i++, fourcc >>= 8) {
+ unsigned char c = fourcc;
+
+ /* Weed out spaces */
+ if (c == ' ')
+ continue;
+
+ /* Print non-control characters as-is */
+ if (c > ' ') {
+ *s = c;
+ s++;
+ continue;
+ }
+
+ if (WARN_ON_ONCE(sizeof(__s) <
+(s - __s) + sizeof(FOURCC_HEX_CHAR_STR)))
+ break;
+
+ *s = '(';
+ s++;
+
[PATCH 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
pixel formats denoted by fourccs. The fourcc encoding is the same for both
so the same implementation can be used.
Suggested-by: Mauro Carvalho Chehab
Signed-off-by: Sakari Ailus
---
Documentation/core-api/printk-formats.rst | 12
lib/test_printf.c | 17 +
lib/vsprintf.c| 86 +++
3 files changed, 115 insertions(+)
diff --git a/Documentation/core-api/printk-formats.rst
b/Documentation/core-api/printk-formats.rst
index 8ebe46b1af39..7aa0451e06fb 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -545,6 +545,18 @@ For printing netdev_features_t.
Passed by reference.
+V4L2 and DRM FourCC code (pixel format)
+---
+
+::
+
+ %p4cc
+
+Print a FourCC code used by V4L2 or DRM, including format endianness and
+its numerical value as hexadecimal.
+
+Passed by reference.
+
Thanks
==
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 2d9f520d2f27..a14754086707 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -624,6 +624,22 @@ static void __init fwnode_pointer(void)
software_node_unregister_nodes(softnodes);
}
+static void __init fourcc_pointer(void)
+{
+ struct {
+ u32 code;
+ char *str;
+ } const try[] = {
+ { 0x20104646, "FF(10) little-endian (0x20104646)", },
+ { 0xa0104646, "FF(10) big-endian (0xa0104646)", },
+ { 0x10111213, "(13)(12)(11)(10) little-endian (0x10111213)", },
+ };
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(try); i++)
+ test(try[i].str, "%p4cc", &try[i].code);
+}
+
static void __init
errptr(void)
{
@@ -668,6 +684,7 @@ test_pointer(void)
flags();
errptr();
fwnode_pointer();
+ fourcc_pointer();
}
static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7c488a1ce318..02e7906619c0 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1721,6 +1721,89 @@ char *netdev_bits(char *buf, char *end, const void *addr,
return special_hex_number(buf, end, num, size);
}
+static noinline_for_stack
+char *fourcc_string(char *buf, char *end, const u32 *__fourcc,
+ struct printf_spec spec, const char *fmt)
+{
+#define FOURCC_HEX_CHAR_STR"(xx)"
+#define FOURCC_BIG_ENDIAN_STR " big-endian"
+#define FOURCC_LITTLE_ENDIAN_STR " little-endian"
+#define FOURCC_HEX_NUMBER " (0x01234567)"
+#define FOURCC_STRING_MAX \
+ FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR \
+ FOURCC_HEX_CHAR_STR FOURCC_LITTLE_ENDIAN_STR FOURCC_HEX_NUMBER
+ struct printf_spec my_spec = {
+ .type = FORMAT_TYPE_UINT,
+ .field_width = 2,
+ .flags = SMALL,
+ .base = 16,
+ .precision = -1,
+ };
+ char __s[sizeof(FOURCC_STRING_MAX)];
+ char *s = __s;
+ unsigned int i;
+ /*
+* The 31st bit defines the endianness of the data, so save its printing
+* for later.
+*/
+ u32 fourcc = *__fourcc & ~BIT(31);
+ int ret;
+
+ if (check_pointer(&buf, end, __fourcc, spec))
+ return buf;
+
+ if (fmt[1] != 'c' || fmt[2] != 'c')
+ return error_string(buf, end, "(%p4?)", spec);
+
+ for (i = 0; i < sizeof(fourcc); i++, fourcc >>= 8) {
+ unsigned char c = fourcc;
+
+ /* Weed out spaces */
+ if (c == ' ')
+ continue;
+
+ /* Print non-control characters as-is */
+ if (c > ' ') {
+ *s = c;
+ s++;
+ continue;
+ }
+
+ if (WARN_ON_ONCE(sizeof(__s) <
+(s - __s) + sizeof(FOURCC_HEX_CHAR_STR)))
+ break;
+
+ *s = '(';
+ s++;
+ s = number(s, s + 2, c, my_spec);
+ *s = ')';
+ s++;
+ }
+
+ ret = strscpy(s, *__fourcc & BIT(31) ? FOURCC_BIG_ENDIAN_STR
+: FOURCC_LITTLE_ENDIAN_STR,
+ sizeof(__s) - (s - __s));
+ if (!WARN_ON_ONCE(ret < 0))
+ s += ret;
+
+ if (!WARN_ON_ONCE(sizeof(__s) <
+ (s - __s) + sizeof(FOURCC_HEX_NUMBER))) {
+ *s = ' ';
+ s++;
+ *s = '(';
+ s++;
+ /* subtract parentheses
Re: [PATCH 04/29] staging: media: ipu3: use vmap instead of reimplementing it
On Tue, Apr 14, 2020 at 03:13:23PM +0200, Christoph Hellwig wrote: > Just use vmap instead of messing with vmalloc internals. > > Signed-off-by: Christoph Hellwig > Acked-by: Peter Zijlstra (Intel) Thanks! Acked-by: Sakari Ailus -- Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Rasmus,
Thanks for the comments.
On Fri, Apr 03, 2020 at 02:10:53PM +0200, Rasmus Villemoes wrote:
> On 03/04/2020 11.11, Sakari Ailus wrote:
> > Add a printk modifier %ppf (for pixel format) for printing V4L2 and DRM
> > pixel formats denoted by 4ccs. The 4cc encoding is the same for both so
> > the same implementation can be used.
>
> This seems quite niche to me, I'm not sure that belongs in vsprintf.c.
> What's wrong with having a
>
> char *fourcc_string(char *buf, u32 x)
>
> that formats x into buf and returns buf, so it can be used in a
>
> char buf[8];
> pr_debug("bla: %s\n", fourcc_string(buf, x))
I guess that could be one option. But changing the implementation could
require changing the size of all those buffers.
We had this approach, too:
https://lore.kernel.org/linux-media/20190916100433.24367-1-hverkuil-ci...@xs4all.nl/>
Let's see if we'll get more comments on this.
>
> Or, for that matter, since it's for debugging, why not just print x with
> 0x%08x?
People generally prefer readable output that they can understand. The codes
are currently being printed in characters, and that's how they are defined
in kernel headers, too. Therefore the hexadecimal values are of secondary
importance (although they could be printed too, as apparently a similar
function in DRM does).
>
> At the very least, the "case '4'" in pointer() should be guarded by
> appropriate CONFIG_*.
>
> Good that Documentation/ gets updated, but test_printf needs updating as
> well.
Agreed.
>
>
> > Suggested-by: Mauro Carvalho Chehab
> > Signed-off-by: Sakari Ailus
> > ---
> > since v1:
> >
> > - Improve documentation (add -BE suffix, refer to "FourCC".
> >
> > - Use '%p4cc' conversion specifier instead of '%ppf'.
>
> Cute. Remember to update the commit log (which still says %ppf).
I will.
>
> > - Fix 31st bit handling in printing FourCC codes.
> >
> > - Use string() correctly, to allow e.g. proper field width handling.
> >
> > - Remove loop, use put_unaligned_le32() instead.
> >
> > Documentation/core-api/printk-formats.rst | 12 +++
> > lib/vsprintf.c| 25 +++
> > 2 files changed, 37 insertions(+)
> >
> > diff --git a/Documentation/core-api/printk-formats.rst
> > b/Documentation/core-api/printk-formats.rst
> > index 8ebe46b1af39..550568520ab6 100644
> > --- a/Documentation/core-api/printk-formats.rst
> > +++ b/Documentation/core-api/printk-formats.rst
> > @@ -545,6 +545,18 @@ For printing netdev_features_t.
> >
> > Passed by reference.
> >
> > +V4L2 and DRM FourCC code (pixel format)
> > +---
> > +
> > +::
> > +
> > + %p4cc
> > +
> > +Print a FourCC code used by V4L2 or DRM. The "-BE" suffix is added on big
> > endian
> > +formats.
> > +
> > +Passed by reference.
>
> Maybe it's obvious to anyone in that business, but perhaps make it more
> clear the 4cc is stored in a u32 (and not, e.g., a __le32 or some other
> integer), that obviously matters when the code treats the pointer as a u32*.
The established practice is to use u32 (as this is really no hardware
involved) but I guess it'd be good to document that here, too.
> > +
> > + put_unaligned_le32(*fourcc & ~BIT(31), s);
> > +
> > + if (*fourcc & BIT(31))
> > + strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > + sizeof(FOURCC_STRING_BE));
>
> put_unaligned_le32(0x0045422d, s + 4) probably generates smaller code,
> and is more in line with building the first part of the string with
> put_unaligned_le32().
Uh. The fourcc code is made of printable characters (apart from the 31st
bit) so it can be printed, but I wouldn't use that here. "-BE" is just a
string and not related to 4ccs.
--
Regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Laurent,
On Sat, Apr 04, 2020 at 03:21:47AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Sat, Apr 04, 2020 at 03:14:25AM +0300, Sakari Ailus wrote:
> > On Fri, Apr 03, 2020 at 09:32:42PM +0300, Andy Shevchenko wrote:
> > > On Fri, Apr 3, 2020 at 8:54 PM Joe Perches wrote:
> > > > On Fri, 2020-04-03 at 19:32 +0200, Mauro Carvalho Chehab wrote:
> > > > > Em Fri, 03 Apr 2020 09:56:42 -0700
> > > > > Joe Perches escreveu:
> > >
> > > > It _might_ be useful to use a CONFIG_MEDIA_SUPPORT guard
> > > > in lib/vsprintf for this.
> > >
> > > No need. FourCC, if Sakari makes it more generic, can be used for
> > > other purposes, e.g. printing component names from the chips (not
> > > related to media at all).
> >
> > Could you elaborate?
> >
> > This could be already used on DRM, presumably, and that does not depend on
> > CONFIG_MEDIA_SUPPORT. I don't know how much there would be a need for that,
> > though, but this remains a possibility.
>
> /**
> * drm_get_format_name - fill a string with a drm fourcc format's name
> * @format: format to compute name of
> * @buf: caller-supplied buffer
> */
> const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf
> *buf)
> {
> snprintf(buf->str, sizeof(buf->str),
>"%c%c%c%c %s-endian (0x%08x)",
>printable_char(format & 0xff),
>printable_char((format >> 8) & 0xff),
>printable_char((format >> 16) & 0xff),
>printable_char((format >> 24) & 0x7f),
>format & DRM_FORMAT_BIG_ENDIAN ? "big" : "little",
>format);
>
> return buf->str;
> }
> EXPORT_SYMBOL(drm_get_format_name);
>
> I'm not advocating for one approach or the other in this case, but we
> should standardize 4CC printing between the two subsystems.
IMO this format (with spaces removed from 4cc) would be fine for V4L2 as
well.
--
Regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Andy, On Fri, Apr 03, 2020 at 09:32:42PM +0300, Andy Shevchenko wrote: > On Fri, Apr 3, 2020 at 8:54 PM Joe Perches wrote: > > On Fri, 2020-04-03 at 19:32 +0200, Mauro Carvalho Chehab wrote: > > > Em Fri, 03 Apr 2020 09:56:42 -0700 > > > Joe Perches escreveu: > > > It _might_ be useful to use a CONFIG_MEDIA_SUPPORT guard > > in lib/vsprintf for this. > > No need. FourCC, if Sakari makes it more generic, can be used for > other purposes, e.g. printing component names from the chips (not > related to media at all). Could you elaborate? This could be already used on DRM, presumably, and that does not depend on CONFIG_MEDIA_SUPPORT. I don't know how much there would be a need for that, though, but this remains a possibility. -- Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Joe,
On Fri, Apr 03, 2020 at 11:38:29AM -0700, Joe Perches wrote:
> On Fri, 2020-04-03 at 13:47 +0300, Sakari Ailus wrote:
> > On Fri, Apr 03, 2020 at 01:24:49PM +0300, Laurent Pinchart wrote:
> > > On Fri, Apr 03, 2020 at 12:11:56PM +0300, Sakari Ailus wrote:
> > > > Add a printk modifier %ppf (for pixel format) for printing V4L2 and DRM
> > > > pixel formats denoted by 4ccs. The 4cc encoding is the same for both so
> > > > the same implementation can be used.
> []
> > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
> > > > +static noinline_for_stack
> > > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > > + struct printf_spec spec, const char *fmt)
> []
> > > > + if (fmt[1] != 'c' || fmt[2] != 'c')
>
> This could check outside a format string if
> the %p4 is at the end of the format string.
>
> printk("%p4", fourcc)
>
> So this should verify
>
> if (!(fmt[1] == 'c' && fmt[2] == 'c'))
How would these be different in functionality? fmt[2] won't be accessed if
fmt[1] is not 'c' (including '\0'), just like on the line above. I find the
original easier to read.
--
Regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Laurent,
Thanks for the comments.
On Fri, Apr 03, 2020 at 01:24:49PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Fri, Apr 03, 2020 at 12:11:56PM +0300, Sakari Ailus wrote:
> > Add a printk modifier %ppf (for pixel format) for printing V4L2 and DRM
> > pixel formats denoted by 4ccs. The 4cc encoding is the same for both so
> > the same implementation can be used.
> >
> > Suggested-by: Mauro Carvalho Chehab
> > Signed-off-by: Sakari Ailus
> > ---
> > since v1:
> >
> > - Improve documentation (add -BE suffix, refer to "FourCC".
> >
> > - Use '%p4cc' conversion specifier instead of '%ppf'.
> >
> > - Fix 31st bit handling in printing FourCC codes.
> >
> > - Use string() correctly, to allow e.g. proper field width handling.
> >
> > - Remove loop, use put_unaligned_le32() instead.
> >
> > Documentation/core-api/printk-formats.rst | 12 +++
> > lib/vsprintf.c| 25 +++
> > 2 files changed, 37 insertions(+)
> >
> > diff --git a/Documentation/core-api/printk-formats.rst
> > b/Documentation/core-api/printk-formats.rst
> > index 8ebe46b1af39..550568520ab6 100644
> > --- a/Documentation/core-api/printk-formats.rst
> > +++ b/Documentation/core-api/printk-formats.rst
> > @@ -545,6 +545,18 @@ For printing netdev_features_t.
> >
> > Passed by reference.
> >
> > +V4L2 and DRM FourCC code (pixel format)
> > +---
> > +
> > +::
> > +
> > + %p4cc
> > +
> > +Print a FourCC code used by V4L2 or DRM. The "-BE" suffix is added on big
> > endian
> > +formats.
> > +
> > +Passed by reference.
> > +
> > Thanks
> > ==
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 7c488a1ce318..93eea6a320da 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1721,6 +1721,28 @@ char *netdev_bits(char *buf, char *end, const void
> > *addr,
> > return special_hex_number(buf, end, num, size);
> > }
> >
> > +static noinline_for_stack
> > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > + struct printf_spec spec, const char *fmt)
> > +{
> > +#define FOURCC_STRING_BE "-BE"
> > + char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
> > +
> > + if (check_pointer(&buf, end, fourcc, spec))
> > + return buf;
> > +
> > + if (fmt[1] != 'c' || fmt[2] != 'c')
> > + return error_string(buf, end, "(%p4?)", spec);
> > +
> > + put_unaligned_le32(*fourcc & ~BIT(31), s);
> > +
> > + if (*fourcc & BIT(31))
> > + strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > + sizeof(FOURCC_STRING_BE));
> > +
> > + return string(buf, end, s, spec);
>
> Taking V4L2_PIX_FMT_Y16_BE as an example, this will print 'Y16 -BE'
> (without quotes). There are other 4CCs that contain spaces and would
> suffer from a similar issue. Even in little-endian format, it would
> result in additional spaces in the output string. Is this what we want ?
> Should the caller always enclose the 4CC in quotes or brackets for
> clarity ? Or should still be done here ?
Good question. Space is indeed a valid character in a 4cc code.
If I omit one or more spaces, it will no longer be a 4cc, but a 3cc or even
a 2cc. Jokes aside, there are probably fair arguments both ways.
I presume there's no 4cc code where the location of a space would make a
difference but all of the spaces are trailing spaces.
It's also worth noting that the formats printed are mostly for debugging
purpose and thus even getting a hypothetical case wrong is not a grave
issue. This would also support just printing them as-is though.
I'm leaning slightly towards omitting any spaces if the code has them. This
is something that couldn't be done by using a macro...
--
Regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Andy,
On Fri, Apr 03, 2020 at 12:54:41PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 03, 2020 at 12:39:16PM +0300, Sakari Ailus wrote:
> > On Fri, Apr 03, 2020 at 12:31:03PM +0300, Andy Shevchenko wrote:
> > > On Fri, Apr 03, 2020 at 12:11:56PM +0300, Sakari Ailus wrote:
> > > > Add a printk modifier %ppf (for pixel format) for printing V4L2 and DRM
> > > > pixel formats denoted by 4ccs. The 4cc encoding is the same for both so
> > > > the same implementation can be used.
> > >
> > > ...
> > >
> > > > +static noinline_for_stack
> > > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > > + struct printf_spec spec, const char *fmt)
> > > > +{
> > >
> > > > +#define FOURCC_STRING_BE "-BE"
> > > > + char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
> > >
> > > I guess it makes it too complicated.
> >
> > The above also clearly binds the size to the data that is expected to
> > contain there. I'd prefer keeping it as-is. And yes, 8 would be correct,
> > too.
>
> OK.
>
> > > char s[8];
> > >
> > > > + if (check_pointer(&buf, end, fourcc, spec))
> > > > + return buf;
> > > > +
> > > > + if (fmt[1] != 'c' || fmt[2] != 'c')
> > > > + return error_string(buf, end, "(%p4?)", spec);
> > > > +
> > >
> > > > + put_unaligned_le32(*fourcc & ~BIT(31), s);
> > >
> > > Can you elaborate what the difference in output with this bit set over
> > > cleared?
> > > I.o.w. why don't we need to put it as BE and for LE case addd "-LE"?
> >
> > The established practice is that big endian formats have "-BE" suffix
> > whereas the little endian ones have nothing. (At least when it comes to
> > V4L2.)
>
> What I meant by the first part of the question is ordering of the characters.
> That ordering of characters is not related to that flag, correct? So, bit
> actually defines the endianess of the data in the certain fourcc.
>
> Probably you need to put a comment to explain this.
How about:
The 31st bit defines the endianness of the data, so save its printing to
the big endian suffix.
>
> > > > + if (*fourcc & BIT(31))
> > > > + strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > > > + sizeof(FOURCC_STRING_BE));
> > >
> > > We know the size, and we may put '\0' as well
> > > if (*fourcc & BIT(31))
> > > strscpy(&s[4], "-BE", sizeof("-BE"));
> > > else
> > > strscpy(&s[4], "", sizeof(""));
> >
> > The rest of the struct memory has already been set to zero in variable
> > declaration.
>
> Which is bogus in my opinion. strscpy() or direct '\0' termination will put it
> more explicit.
There's no need to assign nul a simple character using strscpy(). In that
case I'd just do
s[sizeof(*fourcc)] = '\0';
and remove the initial assignment to zero.
--
Regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Andy,
Thanks for the comments.
On Fri, Apr 03, 2020 at 12:31:03PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 03, 2020 at 12:11:56PM +0300, Sakari Ailus wrote:
> > Add a printk modifier %ppf (for pixel format) for printing V4L2 and DRM
> > pixel formats denoted by 4ccs. The 4cc encoding is the same for both so
> > the same implementation can be used.
>
> ...
>
> > +static noinline_for_stack
> > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > + struct printf_spec spec, const char *fmt)
> > +{
>
> > +#define FOURCC_STRING_BE "-BE"
> > + char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
>
> I guess it makes it too complicated.
The above also clearly binds the size to the data that is expected to
contain there. I'd prefer keeping it as-is. And yes, 8 would be correct,
too.
>
> char s[8];
>
> > + if (check_pointer(&buf, end, fourcc, spec))
> > + return buf;
> > +
> > + if (fmt[1] != 'c' || fmt[2] != 'c')
> > + return error_string(buf, end, "(%p4?)", spec);
> > +
>
> > + put_unaligned_le32(*fourcc & ~BIT(31), s);
>
> Can you elaborate what the difference in output with this bit set over
> cleared?
> I.o.w. why don't we need to put it as BE and for LE case addd "-LE"?
The established practice is that big endian formats have "-BE" suffix
whereas the little endian ones have nothing. (At least when it comes to
V4L2.)
>
> > + if (*fourcc & BIT(31))
> > + strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > + sizeof(FOURCC_STRING_BE));
>
> We know the size, and we may put '\0' as well
> if (*fourcc & BIT(31))
> strscpy(&s[4], "-BE", sizeof("-BE"));
> else
> strscpy(&s[4], "", sizeof(""));
The rest of the struct memory has already been set to zero in variable
declaration.
--
Kind regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Add a printk modifier %ppf (for pixel format) for printing V4L2 and DRM
pixel formats denoted by 4ccs. The 4cc encoding is the same for both so
the same implementation can be used.
Suggested-by: Mauro Carvalho Chehab
Signed-off-by: Sakari Ailus
---
since v1:
- Improve documentation (add -BE suffix, refer to "FourCC".
- Use '%p4cc' conversion specifier instead of '%ppf'.
- Fix 31st bit handling in printing FourCC codes.
- Use string() correctly, to allow e.g. proper field width handling.
- Remove loop, use put_unaligned_le32() instead.
Documentation/core-api/printk-formats.rst | 12 +++
lib/vsprintf.c| 25 +++
2 files changed, 37 insertions(+)
diff --git a/Documentation/core-api/printk-formats.rst
b/Documentation/core-api/printk-formats.rst
index 8ebe46b1af39..550568520ab6 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -545,6 +545,18 @@ For printing netdev_features_t.
Passed by reference.
+V4L2 and DRM FourCC code (pixel format)
+---
+
+::
+
+ %p4cc
+
+Print a FourCC code used by V4L2 or DRM. The "-BE" suffix is added on big
endian
+formats.
+
+Passed by reference.
+
Thanks
==
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7c488a1ce318..93eea6a320da 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1721,6 +1721,28 @@ char *netdev_bits(char *buf, char *end, const void *addr,
return special_hex_number(buf, end, num, size);
}
+static noinline_for_stack
+char *fourcc_string(char *buf, char *end, const u32 *fourcc,
+ struct printf_spec spec, const char *fmt)
+{
+#define FOURCC_STRING_BE "-BE"
+ char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
+
+ if (check_pointer(&buf, end, fourcc, spec))
+ return buf;
+
+ if (fmt[1] != 'c' || fmt[2] != 'c')
+ return error_string(buf, end, "(%p4?)", spec);
+
+ put_unaligned_le32(*fourcc & ~BIT(31), s);
+
+ if (*fourcc & BIT(31))
+ strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
+ sizeof(FOURCC_STRING_BE));
+
+ return string(buf, end, s, spec);
+}
+
static noinline_for_stack
char *address_val(char *buf, char *end, const void *addr,
struct printf_spec spec, const char *fmt)
@@ -2131,6 +2153,7 @@ char *fwnode_string(char *buf, char *end, struct
fwnode_handle *fwnode,
* correctness of the format string and va_list arguments.
* - 'K' For a kernel pointer that should be hidden from unprivileged users
* - 'NF' For a netdev_features_t
+ * - '4cc' V4L2 or DRM FourCC code, with "-BE" suffix on big endian formats.
* - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
*a certain separator (' ' by default):
* C colon
@@ -2223,6 +2246,8 @@ char *pointer(const char *fmt, char *buf, char *end, void
*ptr,
return restricted_pointer(buf, end, ptr, spec);
case 'N':
return netdev_bits(buf, end, ptr, spec, fmt);
+ case '4':
+ return fourcc_string(buf, end, ptr, spec, fmt);
case 'a':
return address_val(buf, end, ptr, spec, fmt);
case 'd':
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Moi, On Thu, Apr 02, 2020 at 11:34:48AM +0300, Jani Nikula wrote: > On Wed, 01 Apr 2020, Sakari Ailus wrote: > > Add a printk modifier %ppf (for pixel format) for printing V4L2 and DRM > > pixel formats denoted by 4ccs. The 4cc encoding is the same for both so > > the same implementation can be used. > > I'm not going to take a strong stand in one way or the other regarding > the patch at hand, but I do think at some point we have to draw a line > what should be included in printk formats. Arguably they should be > reserved to things that are generally useful across large parts of the > kernel, right? > > I think the more specialized you get, the more you should think about > just using the plain old %s, and your own helpers. Because frankly, the > kernel printk specifiers also start getting more than a little obscure. I don't really disagree... While this is functionality very commonly needed in drivers, there are alternatives such as posted here: https://lore.kernel.org/linux-media/20190916100433.24367-1-hverkuil-ci...@xs4all.nl/> The 4cc codes added by this set is still relatively generic (while still Linux subsystem specific and not related to e.g. hardware standards), but I wonder how many other, possibly similar cases there could be in the kernel, and how many new specifiers we might get with those all added. For what it's worth, even C99 defines macros for printing some formats such as PRIu64 for uint64_t. -- Terveisin, Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Andy,
Thanks for the review.
On Wed, Apr 01, 2020 at 06:13:32PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 01, 2020 at 04:13:51PM +0200, Hans Verkuil wrote:
> > On 4/1/20 4:05 PM, Sakari Ailus wrote:
> > > Add a printk modifier %ppf (for pixel format) for printing V4L2 and DRM
> > > pixel formats denoted by 4ccs. The 4cc encoding is the same for both so
> > > the same implementation can be used.
>
> %p4cc ?
Sounds good. Numbers have special handling but AFAIR only right after %
sign, so this should be possible.
>
> > > + char ch[2] = { 0 };
> >
> > This can just be '{ };'
>
> The latter is GCC extension, while above is C standard. Former is slightly
> better I think. Though see below.
>
> > > + unsigned int i;
> > > +
> > > + if (check_pointer(&buf, end, fourcc, spec))
> > > + return buf;
> > > +
> > > + switch (fmt[1]) {
> > > + case 'f':
>
> > > + for (i = 0; i < sizeof(*fourcc); i++) {
> > > + ch[0] = *fourcc >> (i << 3);
> >
> > You need to AND with 0x7f, otherwise a big endian fourcc (bit 31 is set)
> > will look wrong. Also, each character is standard 7 bit ascii, bit 7 isn't
> > used except to indicate a BE variant.
>
> Why not to do it once by a flag and do reset it once?
>
> u32 tmp = *fourcc;
> bool be4cc = tmp & BIT(31);
>
> tmp &= BIT(31);
I had two extra temporary variables in a version I didn't send but I
figured they could be removed. :-)
>
> On top of that, as promised above, why not simple do it in a simpler way, i.e.
> using standard idiom:
>
> for (i = 0; i < sizeof(*fourcc); i++) {
> if (buf < end)
> *buf = tmp >> (i * 8);
> buf++;
> }
> ?
I guess that's at least more efficient, and comparing buf to end is
trivial. I'll do that in v2.
>
> > > + buf = string(buf, end, ch, spec);
> > > + }
> > > +
> > > + if (*fourcc & BIT(31))
> > > + buf = string(buf, end, "-BE", spec);
>
> Another possibility
>
> u8 ch[8];
>
> if (*fourcc & BIT(31)) {
> put_unaligned_be32(tmp, &ch[0]);
> strcpy(&ch[4], "-BE");
> } else {
> put_unaligned_le32(tmp, &ch[0]);
> strcpy(&ch[4], "-LE");
> }
> return string(buf, end, &ch[0], spec);
I think I prefer the loop. I figured you can only call string once,
otherwise field width handling will be broken. Let's see.
>
> > > + return buf;
> > > + default:
> > > + return error_string(buf, end, "(%pp?)", spec);
> > > + }
> > > +}
>
--
Kind regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Hi Hans,
Thank you for the review.
On Wed, Apr 01, 2020 at 04:13:51PM +0200, Hans Verkuil wrote:
> On 4/1/20 4:05 PM, Sakari Ailus wrote:
> > Add a printk modifier %ppf (for pixel format) for printing V4L2 and DRM
> > pixel formats denoted by 4ccs. The 4cc encoding is the same for both so
> > the same implementation can be used.
> >
> > Suggested-by: Mauro Carvalho Chehab
> > Signed-off-by: Sakari Ailus
> > ---
> > Documentation/core-api/printk-formats.rst | 11 +
> > lib/vsprintf.c| 29 +++
> > 2 files changed, 40 insertions(+)
> >
> > diff --git a/Documentation/core-api/printk-formats.rst
> > b/Documentation/core-api/printk-formats.rst
> > index 8ebe46b1af39..b6249f513c09 100644
> > --- a/Documentation/core-api/printk-formats.rst
> > +++ b/Documentation/core-api/printk-formats.rst
> > @@ -545,6 +545,17 @@ For printing netdev_features_t.
> >
> > Passed by reference.
> >
> > +V4L2 and DRM fourcc code (pixel format)
> > +---
> > +
> > +::
> > +
> > + %ppf
> > +
> > +Print a 4cc code used by V4L2 or DRM.
>
> FourCC appears to be the more-or-less official name
> (https://en.wikipedia.org/wiki/FourCC)
>
> I would explain about the -BE suffix for bigendian fourcc variants.
Agreed, I'll address these in v2.
>
> > +
> > +Passed by reference.
> > +
> > Thanks
> > ==
> >
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 7c488a1ce318..b39f0ac317c5 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1721,6 +1721,32 @@ char *netdev_bits(char *buf, char *end, const void
> > *addr,
> > return special_hex_number(buf, end, num, size);
> > }
> >
> > +static noinline_for_stack
> > +char *pixel_format_string(char *buf, char *end, const u32 *fourcc,
> > + struct printf_spec spec, const char *fmt)
> > +{
> > + char ch[2] = { 0 };
>
> This can just be '{ };'
As Andy, I also do prefer { 0 }.
>
> > + unsigned int i;
> > +
> > + if (check_pointer(&buf, end, fourcc, spec))
> > + return buf;
> > +
> > + switch (fmt[1]) {
> > + case 'f':
> > + for (i = 0; i < sizeof(*fourcc); i++) {
> > + ch[0] = *fourcc >> (i << 3);
>
> You need to AND with 0x7f, otherwise a big endian fourcc (bit 31 is set)
> will look wrong. Also, each character is standard 7 bit ascii, bit 7 isn't
> used except to indicate a BE variant.
Good point, will fix for v2.
>
> > + buf = string(buf, end, ch, spec);
> > + }
> > +
> > + if (*fourcc & BIT(31))
> > + buf = string(buf, end, "-BE", spec);
> > +
> > + return buf;
> > + default:
> > + return error_string(buf, end, "(%pp?)", spec);
> > + }
> > +}
> > +
> > static noinline_for_stack
> > char *address_val(char *buf, char *end, const void *addr,
> > struct printf_spec spec, const char *fmt)
> > @@ -2131,6 +2157,7 @@ char *fwnode_string(char *buf, char *end, struct
> > fwnode_handle *fwnode,
> > * correctness of the format string and va_list arguments.
> > * - 'K' For a kernel pointer that should be hidden from unprivileged users
> > * - 'NF' For a netdev_features_t
> > + * - 'pf' V4L2 or DRM pixel format.
>
> I'd say 'FourCC format' instead of 'pixel format'.
Will fix.
--
Regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
Add a printk modifier %ppf (for pixel format) for printing V4L2 and DRM
pixel formats denoted by 4ccs. The 4cc encoding is the same for both so
the same implementation can be used.
Suggested-by: Mauro Carvalho Chehab
Signed-off-by: Sakari Ailus
---
Documentation/core-api/printk-formats.rst | 11 +
lib/vsprintf.c| 29 +++
2 files changed, 40 insertions(+)
diff --git a/Documentation/core-api/printk-formats.rst
b/Documentation/core-api/printk-formats.rst
index 8ebe46b1af39..b6249f513c09 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -545,6 +545,17 @@ For printing netdev_features_t.
Passed by reference.
+V4L2 and DRM fourcc code (pixel format)
+---
+
+::
+
+ %ppf
+
+Print a 4cc code used by V4L2 or DRM.
+
+Passed by reference.
+
Thanks
==
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7c488a1ce318..b39f0ac317c5 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1721,6 +1721,32 @@ char *netdev_bits(char *buf, char *end, const void *addr,
return special_hex_number(buf, end, num, size);
}
+static noinline_for_stack
+char *pixel_format_string(char *buf, char *end, const u32 *fourcc,
+ struct printf_spec spec, const char *fmt)
+{
+ char ch[2] = { 0 };
+ unsigned int i;
+
+ if (check_pointer(&buf, end, fourcc, spec))
+ return buf;
+
+ switch (fmt[1]) {
+ case 'f':
+ for (i = 0; i < sizeof(*fourcc); i++) {
+ ch[0] = *fourcc >> (i << 3);
+ buf = string(buf, end, ch, spec);
+ }
+
+ if (*fourcc & BIT(31))
+ buf = string(buf, end, "-BE", spec);
+
+ return buf;
+ default:
+ return error_string(buf, end, "(%pp?)", spec);
+ }
+}
+
static noinline_for_stack
char *address_val(char *buf, char *end, const void *addr,
struct printf_spec spec, const char *fmt)
@@ -2131,6 +2157,7 @@ char *fwnode_string(char *buf, char *end, struct
fwnode_handle *fwnode,
* correctness of the format string and va_list arguments.
* - 'K' For a kernel pointer that should be hidden from unprivileged users
* - 'NF' For a netdev_features_t
+ * - 'pf' V4L2 or DRM pixel format.
* - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
*a certain separator (' ' by default):
* C colon
@@ -2223,6 +2250,8 @@ char *pointer(const char *fmt, char *buf, char *end, void
*ptr,
return restricted_pointer(buf, end, ptr, spec);
case 'N':
return netdev_bits(buf, end, ptr, spec, fmt);
+ case 'p':
+ return pixel_format_string(buf, end, ptr, spec, fmt);
case 'a':
return address_val(buf, end, ptr, spec, fmt);
case 'd':
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/fourcc: Add bayer formats and modifiers
V4L2. Is that intentional?
Nothing wrong in that though, but is this a rule that's universally
followed?
> +
> /*
> * Format Modifiers:
> *
> @@ -309,6 +365,7 @@ extern "C" {
> #define DRM_FORMAT_MOD_VENDOR_BROADCOM 0x07
> #define DRM_FORMAT_MOD_VENDOR_ARM 0x08
> #define DRM_FORMAT_MOD_VENDOR_ALLWINNER 0x09
> +#define DRM_FORMAT_MOD_VENDOR_MIPI 0x0a
>
> /* add more to the end as needed */
>
> @@ -434,6 +491,17 @@ extern "C" {
> */
> #define I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS fourcc_mod_code(INTEL, 7)
>
> +
> +/*
> + * IPU3 Bayer packing layout
> + *
> + * The IPU3 raw Bayer formats use a custom packing layout where there are no
> + * gaps between each 10-bit sample. It packs 25 pixels into 32 bytes leaving
> + * the 6 most significant bits in the last byte unused. The format is little
> + * endian.
Where is endianness really specified? DRM_FORMAT_BIG_ENDIAN would seem
independent of that.
I might omit endianness definition here.
> + */
> +#define IPU3_FORMAT_MOD_PACKED fourcc_mod_code(INTEL, 8)
> +
> /*
> * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks
> *
> @@ -804,6 +872,33 @@ extern "C" {
> */
> #define DRM_FORMAT_MOD_ALLWINNER_TILED fourcc_mod_code(ALLWINNER, 1)
>
> +/* Mobile Industry Processor Interface (MIPI) modifiers */
> +
> +/*
> + * MIPI CSI-2 packing layout
> + *
> + * The CSI-2 RAW formats (for example Bayer) use a different packing layout
> + * depenindg on the sample size.
"depending"
> + *
> + * - 10-bits per sample
> + * Every four consecutive samples are packed into 5 bytes. Each of the
> first 4
> + * bytes contain the 8 high order bits of the pixels, and the 5th byte
> + * contains the 2 least-significant bits of each pixel, in the same order.
> + *
> + * - 12-bits per sample
> + * Every two consecutive samples are packed into three bytes. Each of the
> + * first two bytes contain the 8 high order bits of the pixels, and the
> third
> + * byte contains the four least-significant bits of each pixel, in the same
> + * order.
> + *
> + * - 14-bits per sample
> + * Every four consecutive samples are packed into seven bytes. Each of the
> + * first four bytes contain the eight high order bits of the pixels, and
> the
> + * three following bytes contains the six least-significant bits of each
> + * pixel, in the same order.
> + */
> +#define MIPI_FORMAT_MOD_CSI2_PACKED fourcc_mod_code(MIPI, 1)
> +
> #if defined(__cplusplus)
> }
> #endif
--
Kind regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 10/12] media: constify fb ops across all drivers
On Tue, Dec 03, 2019 at 06:38:52PM +0200, Jani Nikula wrote: > Now that the fbops member of struct fb_info is const, we can start > making the ops const as well. > > Remove the redundant fbops assignments while at it. > > v2: > - actually add const in vivid > - fix typo (Christophe de Dinechin) > > Cc: Hans Verkuil > Cc: Andy Walls > Cc: linux-me...@vger.kernel.org > Cc: ivtv-de...@ivtvdriver.org > Reviewed-by: Daniel Vetter > Signed-off-by: Jani Nikula Reviewed-by: Sakari Ailus -- Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 11/41] media/v4l2-core/mm: convert put_page() to put_user_page*()
On Tue, Aug 06, 2019 at 06:33:10PM -0700, john.hubb...@gmail.com wrote:
> From: John Hubbard
>
> For pages that were retained via get_user_pages*(), release those pages
> via the new put_user_page*() routines, instead of via put_page() or
> release_pages().
>
> This is part a tree-wide conversion, as described in commit fc1d8e7cca2d
> ("mm: introduce put_user_page*(), placeholder versions").
>
> Cc: Mauro Carvalho Chehab
> Cc: Kees Cook
> Cc: Hans Verkuil
> Cc: Sakari Ailus
> Cc: Jan Kara
> Cc: Robin Murphy
> Cc: Souptick Joarder
> Cc: Dan Williams
> Cc: linux-me...@vger.kernel.org
> Signed-off-by: John Hubbard
Acked-by: Sakari Ailus
--
Sakari Ailus
sakari.ai...@linux.intel.com
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 2/3] media: uapi: Add MEDIA_BUS_FMT_RGB888_3X8 media bus format
On Thu, Jun 06, 2019 at 12:22:46AM +0200, Paul Cercueil wrote: > This patch adds MEDIA_BUS_FMT_RGB888_3X8, used for the GiantPlus > GPM940B0 24-bit TFT panel, where the RGB components are transferred > sequentially on a 8-bit bus. > > Signed-off-by: Paul Cercueil Acked-by: Sakari Ailus -- Sakari Ailus
Re: [PATCH 19/20] lib: image-formats: Add more functions
Hi Maxime,
On Wed, Apr 17, 2019 at 09:54:45AM +0200, Maxime Ripard wrote:
> V4L2 drivers typically need a few more helpers compared to DRM drivers, so
> let's add them.
>
> Signed-off-by: Maxime Ripard
> ---
> include/linux/image-formats.h | 42 -
> 1 file changed, 42 insertions(+)
>
> diff --git a/include/linux/image-formats.h b/include/linux/image-formats.h
> index b78b8e861fc9..a2cf3528bd31 100644
> --- a/include/linux/image-formats.h
> +++ b/include/linux/image-formats.h
> @@ -388,6 +388,48 @@ uint64_t image_format_info_min_pitch(const struct
> image_format_info *info,
> image_format_info_block_height(info, plane));
> }
>
> +/**
> + * image_format_info_plane_stride - determine the stride value
> + * @format: pointer to the image_format_info
> + * @width: plane width
> + * @plane: plane index
> + *
> + * Returns:
> + * The bytes per pixel value for the specified plane.
In V4L2 the bytesperline (stride) value is user configurable and is also
subject to hardware capabilities. So effectively this makes what the
function returns a minimum value for a given format. I think you could add
this to the documentation. This is also not the number of bytes per pixel,
as suggested above. So, e.g.
Returns the minimum number of bytes between two consecutive lines.
> + */
> +static inline
> +unsigned int image_format_info_plane_stride(const struct image_format_info
> *format,
> + unsigned int width, int plane)
> +{
> + if (!format || plane >= format->num_planes)
> + return 0;
> +
> + return image_format_info_plane_width(format, width, plane) *
> + image_format_info_plane_cpp(format, plane);
> +}
> +
> +/**
> + * image_format_info_plane_size - determine the size value
> + * @format: pointer to the image_format_info
> + * @width: plane width
> + * @height: plane width
> + * @plane: plane index
> + *
> + * Returns:
> + * The size of the plane buffer.
Similarly:
Returns the minimum size of the plane buffer.
With these,
Acked-by: Sakari Ailus
> + */
> +static inline
> +unsigned int image_format_info_plane_size(const struct image_format_info
> *format,
> + unsigned int width, unsigned int
> height,
> + int plane)
> +{
> + if (!format || plane >= format->num_planes)
> + return 0;
> +
> + return image_format_info_plane_stride(format, width, plane) *
> + image_format_info_plane_height(format, height, plane);
> +}
> +
> const struct image_format_info *__image_format_drm_lookup(u32 drm);
> const struct image_format_info *__image_format_v4l2_lookup(u32 v4l2);
> const struct image_format_info *image_format_drm_lookup(u32 drm);
--
Sakari Ailus
sakari.ai...@linux.intel.com
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] lib/scatterlist: Provide a DMA page iterator
On Fri, Jan 04, 2019 at 10:35:43PM +, Jason Gunthorpe wrote:
> Commit 2db76d7c3c6d ("lib/scatterlist: sg_page_iter: support sg lists w/o
> backing pages") introduced the sg_page_iter_dma_address() function without
> providing a way to use it in the general case. If the sg_dma_len is not
> equal to the dma_length callers cannot safely use the
> for_each_sg_page/sg_page_iter_dma_address combination.
>
> Resolve this API mistake by providing a DMA specific iterator,
> for_each_sg_dma_page(), that uses the right length so
> sg_page_iter_dma_address() works as expected with all sglists. A new
> iterator type is introduced to provide compile-time safety against wrongly
> mixing accessors and iterators.
>
> Signed-off-by: Jason Gunthorpe
Acked-by: Sakari Ailus (ipu3-cio2)
--
Sakari Ailus
sakari.ai...@linux.intel.com
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 8/9] phy: Add Cadence D-PHY support
On Wed, Jan 09, 2019 at 10:33:25AM +0100, Maxime Ripard wrote: > Cadence has designed a D-PHY that can be used by the, currently in tree, > DSI bridge (DRM), CSI Transceiver and CSI Receiver (v4l2) drivers. > > Only the DSI driver has an ad-hoc driver for that phy at the moment, while > the v4l2 drivers are completely missing any phy support. In order to make > that phy support available to all these drivers, without having to > duplicate that code three times, let's create a generic phy framework > driver. > > Signed-off-by: Maxime Ripard Reviewed-by: Sakari Ailus -- Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 7/9] dt-bindings: phy: Move the Cadence D-PHY bindings
Hi Maxime,
On Wed, Jan 09, 2019 at 10:33:24AM +0100, Maxime Ripard wrote:
> The Cadence D-PHY bindings was defined as part of the DSI block so far.
> However, since it's now going to be a separate driver, we need to move the
> binding to a file of its own.
>
> Signed-off-by: Maxime Ripard
Reviewed-by: Sakari Ailus
Could you also send this to the devicetree list, please?
> ---
> Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt | 21 +---
> Documentation/devicetree/bindings/phy/cdns,dphy.txt | 20 +++-
> 2 files changed, 20 insertions(+), 21 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/phy/cdns,dphy.txt
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> index f5725bb6c61c..525a4bfd8634 100644
> --- a/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/cdns,dsi.txt
> @@ -31,28 +31,7 @@ Required subnodes:
> - one subnode per DSI device connected on the DSI bus. Each DSI device should
>contain a reg property encoding its virtual channel.
>
> -Cadence DPHY
> -
> -
> -Cadence DPHY block.
> -
> -Required properties:
> -- compatible: should be set to "cdns,dphy".
> -- reg: physical base address and length of the DPHY registers.
> -- clocks: DPHY reference clocks.
> -- clock-names: must contain "psm" and "pll_ref".
> -- #phy-cells: must be set to 0.
> -
> -
> Example:
> - dphy0: dphy@fd0e{
> - compatible = "cdns,dphy";
> - reg = <0x0 0xfd0e 0x0 0x1000>;
> - clocks = <&psm_clk>, <&pll_ref_clk>;
> - clock-names = "psm", "pll_ref";
> - #phy-cells = <0>;
> - };
> -
> dsi0: dsi@fd0c {
> compatible = "cdns,dsi";
> reg = <0x0 0xfd0c 0x0 0x1000>;
> diff --git a/Documentation/devicetree/bindings/phy/cdns,dphy.txt
> b/Documentation/devicetree/bindings/phy/cdns,dphy.txt
> new file mode 100644
> index ..1095bc4e72d9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/cdns,dphy.txt
> @@ -0,0 +1,20 @@
> +Cadence DPHY
> +
> +
> +Cadence DPHY block.
> +
> +Required properties:
> +- compatible: should be set to "cdns,dphy".
> +- reg: physical base address and length of the DPHY registers.
> +- clocks: DPHY reference clocks.
> +- clock-names: must contain "psm" and "pll_ref".
> +- #phy-cells: must be set to 0.
> +
> +Example:
> + dphy0: dphy@fd0e{
> + compatible = "cdns,dphy";
> + reg = <0x0 0xfd0e 0x0 0x1000>;
> + clocks = <&psm_clk>, <&pll_ref_clk>;
> + clock-names = "psm", "pll_ref";
> + #phy-cells = <0>;
> + };
> --
> git-series 0.9.1
--
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 0/9] phy: Add configuration interface for MIPI D-PHY devices
On Wed, Jan 09, 2019 at 10:33:17AM +0100, Maxime Ripard wrote: > Hi, > > Here is a set of patches to allow the phy framework consumers to test and > apply runtime configurations. > > This is needed to support more phy classes that require tuning based on > parameters depending on the current use case of the device, in addition to > the power state management already provided by the current functions. > > A first test bed for that API are the MIPI D-PHY devices. There's a number > of solutions that have been used so far to support these phy, most of the > time being an ad-hoc driver in the consumer. > > That approach has a big shortcoming though, which is that this is quite > difficult to deal with consumers integrated with multiple variants of phy, > of multiple consumers integrated with the same phy. > > The latter case can be found in the Cadence DSI bridge, and the CSI > transceiver and receivers. All of them are integrated with the same phy, or > can be integrated with different phy, depending on the implementation. > > I've looked at all the MIPI DSI drivers I could find, and gathered all the > parameters I could find. The interface should be complete, and most of the > drivers can be converted in the future. The current set converts two of > them: the above mentionned Cadence DSI driver so that the v4l2 drivers can > use them, and the Allwinner MIPI-DSI driver. > > Let me know what you think, > Maxime > > Changes from v3 > - Rebased on 5.0-rc1 > - Added the fixes suggested by Sakari Thanks! For patches 1--3: Acked-by: Sakari Ailus -- Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 03/10] phy: Add MIPI D-PHY configuration options
Hi Maxime, On Mon, Dec 17, 2018 at 04:49:21PM +0100, Maxime Ripard wrote: > Hi Sakari, > > Thanks for your feedback. > > On Thu, Dec 13, 2018 at 10:49:28PM +0200, sakari.ai...@iki.fi wrote: > > > + /** > > > + * @lanes: > > > + * > > > + * Number of active data lanes used for the transmissions. > > > > Could you add that these are the first "lanes" number of lanes from what > > are available? > > I'm not quite sure I understood this part though, what did you mean? A number of lanes are routed between the two devices on hardware, and this field is specifying how many of them are in use. In order for the bus to function, both ends need to be in agreement on which of these lanes are actually being used. The current practice I've seen without exceptions is that these are the first n lanes. -- Regards, Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 03/10] phy: Add MIPI D-PHY configuration options
Hi Maxime, One more small note. On Fri, Dec 07, 2018 at 02:55:30PM +0100, Maxime Ripard wrote: ... > + /** > + * @wakeup: > + * > + * Time, in picoseconds, that a transmitter drives a Mark-1 > + * state prior to a Stop state in order to initiate an exit > + * from ULPS. > + * > + * Minimum value: 10 ps > + */ > + unsigned intwakeup; This is very close to the higher limit of the type's value range. How about using ns or µs for this one? -- Sakari Ailus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
