diff mbox series

[PATCHv2,01/10] drm/crtc: Add histogram properties

Message ID 20241204091456.1785858-1-arun.r.murthy@intel.com (mailing list archive)
State New
Headers show
Series [PATCHv2,01/10] drm/crtc: Add histogram properties | expand

Commit Message

Arun R Murthy Dec. 4, 2024, 9:14 a.m. UTC
Add variables for histogram drm_property, its corrsponding crtc_state
variables and define the structure pointed by the blob property.
struct drm_histogram defined in include/uapi/drm/drm_mode.h
The blob data pointer will be the address of the struct drm_histogram.
The struct drm_histogram will be used for both reading the histogram and
writing the image enhanced data.
struct drm_histogram {
	u64 data_ptr;
	u32 nr_elements;
}
The element data_ptr holds the address of the histogram statistics data
and 'nr_elements' represents the number of elements pointed by the
pointer held in data_ptr.
The same element data_ptr in teh struct drm_histogram when writing the
image enahnced data from user to KMD holds the address to pixel factor.

v2: Added blob description in commit message (Dmitry)

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 include/drm/drm_crtc.h      | 48 +++++++++++++++++++++++++++++++++++++
 include/uapi/drm/drm_mode.h | 11 +++++++++
 2 files changed, 59 insertions(+)

Comments

Dmitry Baryshkov Dec. 4, 2024, 11:47 a.m. UTC | #1
On Wed, Dec 04, 2024 at 02:44:56PM +0530, Arun R Murthy wrote:
> Add variables for histogram drm_property, its corrsponding crtc_state
> variables and define the structure pointed by the blob property.
> struct drm_histogram defined in include/uapi/drm/drm_mode.h
> The blob data pointer will be the address of the struct drm_histogram.
> The struct drm_histogram will be used for both reading the histogram and
> writing the image enhanced data.
> struct drm_histogram {
> 	u64 data_ptr;
> 	u32 nr_elements;
> }
> The element data_ptr holds the address of the histogram statistics data
> and 'nr_elements' represents the number of elements pointed by the
> pointer held in data_ptr.
> The same element data_ptr in teh struct drm_histogram when writing the
> image enahnced data from user to KMD holds the address to pixel factor.
> 
> v2: Added blob description in commit message (Dmitry)

No, it is not a proper description. What is the actual data format
inside the blob? If I were to implement drm_histogram support for e.g.
VKMS, what kind of data should the driver generate? What is the format
of the response data from the userspace app? The ABI description should
allow an independent but completely compatible implementation to be
written from scratch.

BTW: something is really wrong with the way you are sending the
patchset. For this iteration I see only two patches with no threading.
Please stop playing with individual versions of the patches, generate
and send the patchset via a single invocation of git send-email (or git
format-patches / git send-email). The version is a version of the
patchset, not some other number. You can use the b4 tool which can
handle everything for you.

> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  include/drm/drm_crtc.h      | 48 +++++++++++++++++++++++++++++++++++++
>  include/uapi/drm/drm_mode.h | 11 +++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8b48a1974da3..3984cfa00cbf 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -274,6 +274,38 @@ struct drm_crtc_state {
>  	 */
>  	struct drm_property_blob *gamma_lut;
>  
> +	/**
> +	 * @histogram_enable:
> +	 *
> +	 * This will be set if histogram is enabled for the CRTC.
> +	 */
> +	bool histogram_enable;
> +
> +	/**
> +	 * @histogram_data:
> +	 *
> +	 * This will hold the pointer to the struct drm_histogram.
> +	 * The element data in drm_histogram will hold the pointer to the
> +	 * histogram data generated by the hardware.
> +	 */
> +	struct drm_property_blob *histogram_data;
> +
> +	/**
> +	 * @histogram_-iet:
> +	 *
> +	 * This will hold the pointer to the struct drm_histogram.
> +	 * The element data in drm_histogram will hold the pointer to the
> +	 * histogram image enhancement generated by the algorithm that is to
> +	 * be fed back to the hardware.
> +	 */
> +	struct drm_property_blob *histogram_iet;
> +	/**
> +	 * @histogram_iet_updates:
> +	 *
> +	 * Convey that the image enhanced data has been updated by the user
> +	 */
> +	bool histogram_iet_updated;
> +
>  	/**
>  	 * @target_vblank:
>  	 *
> @@ -1088,6 +1120,22 @@ struct drm_crtc {
>  	 */
>  	struct drm_property *scaling_filter_property;
>  
> +	/**
> +	 * @histogram_enable_property: Optional CRTC property for enabling or
> +	 * disabling global histogram.
> +	 */
> +	struct drm_property *histogram_enable_property;
> +	/**
> +	 * @histogram_data_proeprty: Optional CRTC property for getting the
> +	 * histogram blob data.
> +	 */
> +	struct drm_property *histogram_data_property;
> +	/**
> +	 * @histogram_iet_proeprty: Optional CRTC property for writing the
> +	 * image enhanced blob data
> +	 */
> +	struct drm_property *histogram_iet_property;
> +
>  	/**
>  	 * @state:
>  	 *
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index c082810c08a8..da4396f57ed1 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -1355,6 +1355,17 @@ struct drm_mode_closefb {
>  	__u32 pad;
>  };
>  
> +/**
> + * struct drm_histogram
> + * data_ptr: pointer to the array fo u32 data. This data can be histogram
> + * raw data or image enhanced data
> + * nr_elements: number of elements pointed by the data @data_ptr
> + */
> +struct drm_histogram {
> +	__u64 data_ptr;
> +	__u32 nr_elements;
> +};
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> -- 
> 2.25.1
>
Arun R Murthy Dec. 5, 2024, 7:58 a.m. UTC | #2
> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Sent: Wednesday, December 4, 2024 5:17 PM
> To: Murthy, Arun R <arun.r.murthy@intel.com>
> Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; dri-
> devel@lists.freedesktop.org
> Subject: Re: [PATCHv2 01/10] drm/crtc: Add histogram properties
> 
> On Wed, Dec 04, 2024 at 02:44:56PM +0530, Arun R Murthy wrote:
> > Add variables for histogram drm_property, its corrsponding crtc_state
> > variables and define the structure pointed by the blob property.
> > struct drm_histogram defined in include/uapi/drm/drm_mode.h The blob
> > data pointer will be the address of the struct drm_histogram.
> > The struct drm_histogram will be used for both reading the histogram
> > and writing the image enhanced data.
> > struct drm_histogram {
> > 	u64 data_ptr;
> > 	u32 nr_elements;
> > }
> > The element data_ptr holds the address of the histogram statistics
> > data and 'nr_elements' represents the number of elements pointed by
> > the pointer held in data_ptr.
> > The same element data_ptr in teh struct drm_histogram when writing the
> > image enahnced data from user to KMD holds the address to pixel factor.
> >
> > v2: Added blob description in commit message (Dmitry)
> 
> No, it is not a proper description. What is the actual data format inside the
> blob? If I were to implement drm_histogram support for e.g.
> VKMS, what kind of data should the driver generate? What is the format of the
> response data from the userspace app? The ABI description should allow an
> independent but completely compatible implementation to be written from
> scratch.
> 
The histogram is generated by the hardware.
Histogram represents integer which is the pixel count and when it comes to 
the IET(Image Enhancement) to be written back to hardware its again an
integer, pixel factor.
Is this the information that you expected to be added or something else?

> BTW: something is really wrong with the way you are sending the patchset. For
> this iteration I see only two patches with no threading.
> Please stop playing with individual versions of the patches, generate and send
> the patchset via a single invocation of git send-email (or git format-patches / git
> send-email). The version is a version of the patchset, not some other number.
> You can use the b4 tool which can handle everything for you.
Sorry, will send the entire patchset in future.

Thanks and Regards,
Arun R Murthy
-------------------
Dmitry Baryshkov Dec. 5, 2024, 11:48 a.m. UTC | #3
On Thu, 5 Dec 2024 at 09:58, Murthy, Arun R <arun.r.murthy@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Sent: Wednesday, December 4, 2024 5:17 PM
> > To: Murthy, Arun R <arun.r.murthy@intel.com>
> > Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; dri-
> > devel@lists.freedesktop.org
> > Subject: Re: [PATCHv2 01/10] drm/crtc: Add histogram properties
> >
> > On Wed, Dec 04, 2024 at 02:44:56PM +0530, Arun R Murthy wrote:
> > > Add variables for histogram drm_property, its corrsponding crtc_state
> > > variables and define the structure pointed by the blob property.
> > > struct drm_histogram defined in include/uapi/drm/drm_mode.h The blob
> > > data pointer will be the address of the struct drm_histogram.
> > > The struct drm_histogram will be used for both reading the histogram
> > > and writing the image enhanced data.
> > > struct drm_histogram {
> > >     u64 data_ptr;
> > >     u32 nr_elements;
> > > }
> > > The element data_ptr holds the address of the histogram statistics
> > > data and 'nr_elements' represents the number of elements pointed by
> > > the pointer held in data_ptr.
> > > The same element data_ptr in teh struct drm_histogram when writing the
> > > image enahnced data from user to KMD holds the address to pixel factor.
> > >
> > > v2: Added blob description in commit message (Dmitry)
> >
> > No, it is not a proper description. What is the actual data format inside the
> > blob? If I were to implement drm_histogram support for e.g.
> > VKMS, what kind of data should the driver generate? What is the format of the
> > response data from the userspace app? The ABI description should allow an
> > independent but completely compatible implementation to be written from
> > scratch.
> >
> The histogram is generated by the hardware.
> Histogram represents integer which is the pixel count and when it comes to
> the IET(Image Enhancement) to be written back to hardware its again an
> integer, pixel factor.
> Is this the information that you expected to be added or something else?

You are defining the interface between the kernel and userspace. The
interface should be defined in a way that allows us (developers) to
understand the data, make a decision whether it fits a generic
namespace (which means that other drivers must be able to implement
the same interface), it fits namespace specific to i915 / Xe (then we
will have platform-specific properties for the feature that might be
implemented by other platforms) or it doesn't fit anything at all.

So the documentation must contain the specification of the binary data
inside the blobs. An IGT, modetest or some other compositor must be
able to parse the data and generate (some) response without using your
library.

>
> > BTW: something is really wrong with the way you are sending the patchset. For
> > this iteration I see only two patches with no threading.
> > Please stop playing with individual versions of the patches, generate and send
> > the patchset via a single invocation of git send-email (or git format-patches / git
> > send-email). The version is a version of the patchset, not some other number.
> > You can use the b4 tool which can handle everything for you.
> Sorry, will send the entire patchset in future.
>
> Thanks and Regards,
> Arun R Murthy
> -------------------
Arun R Murthy Dec. 5, 2024, 4:29 p.m. UTC | #4
> > > -----Original Message-----
> > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > Sent: Wednesday, December 4, 2024 5:17 PM
> > > To: Murthy, Arun R <arun.r.murthy@intel.com>
> > > Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org;
> > > dri- devel@lists.freedesktop.org
> > > Subject: Re: [PATCHv2 01/10] drm/crtc: Add histogram properties
> > >
> > > On Wed, Dec 04, 2024 at 02:44:56PM +0530, Arun R Murthy wrote:
> > > > Add variables for histogram drm_property, its corrsponding
> > > > crtc_state variables and define the structure pointed by the blob property.
> > > > struct drm_histogram defined in include/uapi/drm/drm_mode.h The
> > > > blob data pointer will be the address of the struct drm_histogram.
> > > > The struct drm_histogram will be used for both reading the
> > > > histogram and writing the image enhanced data.
> > > > struct drm_histogram {
> > > >     u64 data_ptr;
> > > >     u32 nr_elements;
> > > > }
> > > > The element data_ptr holds the address of the histogram statistics
> > > > data and 'nr_elements' represents the number of elements pointed
> > > > by the pointer held in data_ptr.
> > > > The same element data_ptr in teh struct drm_histogram when writing
> > > > the image enahnced data from user to KMD holds the address to pixel
> factor.
> > > >
> > > > v2: Added blob description in commit message (Dmitry)
> > >
> > > No, it is not a proper description. What is the actual data format
> > > inside the blob? If I were to implement drm_histogram support for e.g.
> > > VKMS, what kind of data should the driver generate? What is the
> > > format of the response data from the userspace app? The ABI
> > > description should allow an independent but completely compatible
> > > implementation to be written from scratch.
> > >
> > The histogram is generated by the hardware.
> > Histogram represents integer which is the pixel count and when it
> > comes to the IET(Image Enhancement) to be written back to hardware its
> > again an integer, pixel factor.
> > Is this the information that you expected to be added or something else?
> 
> You are defining the interface between the kernel and userspace. The interface
> should be defined in a way that allows us (developers) to understand the data,
> make a decision whether it fits a generic namespace (which means that other
> drivers must be able to implement the same interface), it fits namespace
> specific to i915 / Xe (then we will have platform-specific properties for the
> feature that might be implemented by other platforms) or it doesn't fit
> anything at all.
> 
Sure will add the above information in the commit message and also in the kernel doc.
If there are no other review comments, then I will push the next version of patch implementing your comments.

> So the documentation must contain the specification of the binary data inside
> the blobs. An IGT, modetest or some other compositor must be able to parse
> the data and generate (some) response without using your library.
> 
IGT patch can be located at https://patchwork.freedesktop.org/series/135789/ This include test with and without library.
The corresponding compositor changes can be located at https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3873/diffs?commit_id=270808ca7c8be48513553d95b4a47541f5d40206

Thanks and Regards,
Arun R Murthy
--------------------
Dmitry Baryshkov Dec. 5, 2024, 7:39 p.m. UTC | #5
On Thu, Dec 05, 2024 at 04:29:55PM +0000, Murthy, Arun R wrote:
> > > > -----Original Message-----
> > > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > Sent: Wednesday, December 4, 2024 5:17 PM
> > > > To: Murthy, Arun R <arun.r.murthy@intel.com>
> > > > Cc: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org;
> > > > dri- devel@lists.freedesktop.org
> > > > Subject: Re: [PATCHv2 01/10] drm/crtc: Add histogram properties
> > > >
> > > > On Wed, Dec 04, 2024 at 02:44:56PM +0530, Arun R Murthy wrote:
> > > > > Add variables for histogram drm_property, its corrsponding
> > > > > crtc_state variables and define the structure pointed by the blob property.
> > > > > struct drm_histogram defined in include/uapi/drm/drm_mode.h The
> > > > > blob data pointer will be the address of the struct drm_histogram.
> > > > > The struct drm_histogram will be used for both reading the
> > > > > histogram and writing the image enhanced data.
> > > > > struct drm_histogram {
> > > > >     u64 data_ptr;
> > > > >     u32 nr_elements;
> > > > > }
> > > > > The element data_ptr holds the address of the histogram statistics
> > > > > data and 'nr_elements' represents the number of elements pointed
> > > > > by the pointer held in data_ptr.
> > > > > The same element data_ptr in teh struct drm_histogram when writing
> > > > > the image enahnced data from user to KMD holds the address to pixel
> > factor.
> > > > >
> > > > > v2: Added blob description in commit message (Dmitry)
> > > >
> > > > No, it is not a proper description. What is the actual data format
> > > > inside the blob? If I were to implement drm_histogram support for e.g.
> > > > VKMS, what kind of data should the driver generate? What is the
> > > > format of the response data from the userspace app? The ABI
> > > > description should allow an independent but completely compatible
> > > > implementation to be written from scratch.
> > > >
> > > The histogram is generated by the hardware.
> > > Histogram represents integer which is the pixel count and when it
> > > comes to the IET(Image Enhancement) to be written back to hardware its
> > > again an integer, pixel factor.
> > > Is this the information that you expected to be added or something else?
> > 
> > You are defining the interface between the kernel and userspace. The interface
> > should be defined in a way that allows us (developers) to understand the data,
> > make a decision whether it fits a generic namespace (which means that other
> > drivers must be able to implement the same interface), it fits namespace
> > specific to i915 / Xe (then we will have platform-specific properties for the
> > feature that might be implemented by other platforms) or it doesn't fit
> > anything at all.
> > 
> Sure will add the above information in the commit message and also in the kernel doc.
> If there are no other review comments, then I will push the next version of patch implementing your comments.
> 
> > So the documentation must contain the specification of the binary data inside
> > the blobs. An IGT, modetest or some other compositor must be able to parse
> > the data and generate (some) response without using your library.
> > 
> IGT patch can be located at https://patchwork.freedesktop.org/series/135789/ This include test with and without library.
> The corresponding compositor changes can be located at https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/3873/diffs?commit_id=270808ca7c8be48513553d95b4a47541f5d40206

I know. But that's not a replacement for the documentation. Can I
implement an alternative implementation without using your library?

> 
> Thanks and Regards,
> Arun R Murthy
> --------------------
Arun R Murthy Dec. 6, 2024, 10:33 a.m. UTC | #6
> On Thu, Dec 05, 2024 at 04:29:55PM +0000, Murthy, Arun R wrote:
> > > > > -----Original Message-----
> > > > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > Sent: Wednesday, December 4, 2024 5:17 PM
> > > > > To: Murthy, Arun R <arun.r.murthy@intel.com>
> > > > > Cc: intel-xe@lists.freedesktop.org;
> > > > > intel-gfx@lists.freedesktop.org;
> > > > > dri- devel@lists.freedesktop.org
> > > > > Subject: Re: [PATCHv2 01/10] drm/crtc: Add histogram properties
> > > > >
> > > > > On Wed, Dec 04, 2024 at 02:44:56PM +0530, Arun R Murthy wrote:
> > > > > > Add variables for histogram drm_property, its corrsponding
> > > > > > crtc_state variables and define the structure pointed by the blob
> property.
> > > > > > struct drm_histogram defined in include/uapi/drm/drm_mode.h
> > > > > > The blob data pointer will be the address of the struct drm_histogram.
> > > > > > The struct drm_histogram will be used for both reading the
> > > > > > histogram and writing the image enhanced data.
> > > > > > struct drm_histogram {
> > > > > >     u64 data_ptr;
> > > > > >     u32 nr_elements;
> > > > > > }
> > > > > > The element data_ptr holds the address of the histogram
> > > > > > statistics data and 'nr_elements' represents the number of
> > > > > > elements pointed by the pointer held in data_ptr.
> > > > > > The same element data_ptr in teh struct drm_histogram when
> > > > > > writing the image enahnced data from user to KMD holds the
> > > > > > address to pixel
> > > factor.
> > > > > >
> > > > > > v2: Added blob description in commit message (Dmitry)
> > > > >
> > > > > No, it is not a proper description. What is the actual data
> > > > > format inside the blob? If I were to implement drm_histogram support
> for e.g.
> > > > > VKMS, what kind of data should the driver generate? What is the
> > > > > format of the response data from the userspace app? The ABI
> > > > > description should allow an independent but completely
> > > > > compatible implementation to be written from scratch.
> > > > >
> > > > The histogram is generated by the hardware.
> > > > Histogram represents integer which is the pixel count and when it
> > > > comes to the IET(Image Enhancement) to be written back to hardware
> > > > its again an integer, pixel factor.
> > > > Is this the information that you expected to be added or something else?
> > >
> > > You are defining the interface between the kernel and userspace. The
> > > interface should be defined in a way that allows us (developers) to
> > > understand the data, make a decision whether it fits a generic
> > > namespace (which means that other drivers must be able to implement
> > > the same interface), it fits namespace specific to i915 / Xe (then
> > > we will have platform-specific properties for the feature that might
> > > be implemented by other platforms) or it doesn't fit anything at all.
> > >
> > Sure will add the above information in the commit message and also in the
> kernel doc.
> > If there are no other review comments, then I will push the next version of
> patch implementing your comments.
> >
> > > So the documentation must contain the specification of the binary
> > > data inside the blobs. An IGT, modetest or some other compositor
> > > must be able to parse the data and generate (some) response without using
> your library.
> > >
> > IGT patch can be located at
> https://patchwork.freedesktop.org/series/135789/ This include test with and
> without library.
> > The corresponding compositor changes can be located at
> > https://gitlab.gnome.org/GNOME/mutter/-
> /merge_requests/3873/diffs?comm
> > it_id=270808ca7c8be48513553d95b4a47541f5d40206
> 
> I know. But that's not a replacement for the documentation. Can I implement
> an alternative implementation without using your library?
> 
Yes, exposure of the properties is being done in this series and also support
from hardware is being done. The library/algorithm is open and the
community is free to add their own and make use of the histogram and the
image enhancement feature posted in this series.

Thanks and Regards,
Arun R Murthy
-------------------
Dmitry Baryshkov Dec. 7, 2024, 12:59 p.m. UTC | #7
On 6 December 2024 12:33:38 EET, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
>> On Thu, Dec 05, 2024 at 04:29:55PM +0000, Murthy, Arun R wrote:
>> > > > > -----Original Message-----
>> > > > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> > > > > Sent: Wednesday, December 4, 2024 5:17 PM
>> > > > > To: Murthy, Arun R <arun.r.murthy@intel.com>
>> > > > > Cc: intel-xe@lists.freedesktop.org;
>> > > > > intel-gfx@lists.freedesktop.org;
>> > > > > dri- devel@lists.freedesktop.org
>> > > > > Subject: Re: [PATCHv2 01/10] drm/crtc: Add histogram properties
>> > > > >
>> > > > > On Wed, Dec 04, 2024 at 02:44:56PM +0530, Arun R Murthy wrote:
>> > > > > > Add variables for histogram drm_property, its corrsponding
>> > > > > > crtc_state variables and define the structure pointed by the blob
>> property.
>> > > > > > struct drm_histogram defined in include/uapi/drm/drm_mode.h
>> > > > > > The blob data pointer will be the address of the struct drm_histogram.
>> > > > > > The struct drm_histogram will be used for both reading the
>> > > > > > histogram and writing the image enhanced data.
>> > > > > > struct drm_histogram {
>> > > > > >     u64 data_ptr;
>> > > > > >     u32 nr_elements;
>> > > > > > }
>> > > > > > The element data_ptr holds the address of the histogram
>> > > > > > statistics data and 'nr_elements' represents the number of
>> > > > > > elements pointed by the pointer held in data_ptr.
>> > > > > > The same element data_ptr in teh struct drm_histogram when
>> > > > > > writing the image enahnced data from user to KMD holds the
>> > > > > > address to pixel
>> > > factor.
>> > > > > >
>> > > > > > v2: Added blob description in commit message (Dmitry)
>> > > > >
>> > > > > No, it is not a proper description. What is the actual data
>> > > > > format inside the blob? If I were to implement drm_histogram support
>> for e.g.
>> > > > > VKMS, what kind of data should the driver generate? What is the
>> > > > > format of the response data from the userspace app? The ABI
>> > > > > description should allow an independent but completely
>> > > > > compatible implementation to be written from scratch.
>> > > > >
>> > > > The histogram is generated by the hardware.
>> > > > Histogram represents integer which is the pixel count and when it
>> > > > comes to the IET(Image Enhancement) to be written back to hardware
>> > > > its again an integer, pixel factor.
>> > > > Is this the information that you expected to be added or something else?
>> > >
>> > > You are defining the interface between the kernel and userspace. The
>> > > interface should be defined in a way that allows us (developers) to
>> > > understand the data, make a decision whether it fits a generic
>> > > namespace (which means that other drivers must be able to implement
>> > > the same interface), it fits namespace specific to i915 / Xe (then
>> > > we will have platform-specific properties for the feature that might
>> > > be implemented by other platforms) or it doesn't fit anything at all.
>> > >
>> > Sure will add the above information in the commit message and also in the
>> kernel doc.
>> > If there are no other review comments, then I will push the next version of
>> patch implementing your comments.
>> >
>> > > So the documentation must contain the specification of the binary
>> > > data inside the blobs. An IGT, modetest or some other compositor
>> > > must be able to parse the data and generate (some) response without using
>> your library.
>> > >
>> > IGT patch can be located at
>> https://patchwork.freedesktop.org/series/135789/ This include test with and
>> without library.
>> > The corresponding compositor changes can be located at
>> > https://gitlab.gnome.org/GNOME/mutter/-
>> /merge_requests/3873/diffs?comm
>> > it_id=270808ca7c8be48513553d95b4a47541f5d40206
>> 
>> I know. But that's not a replacement for the documentation. Can I implement
>> an alternative implementation without using your library?
>> 
>Yes, exposure of the properties is being done in this series and also support
>from hardware is being done. The library/algorithm is open and the
>community is free to add their own and make use of the histogram and the
>image enhancement feature posted in this series

Please excuse me if I am not clear enough. Please provide a description of the data format used by these properties. If you need a criteria whether to consider the specification to be good enough, here is one for you: by using only the spec it should be possible to implement histogram support, compatible with your library, in the vkms driver. Anything else is underdocumented. I don't think that we can accept uABI based on the undocumented blobs.

>
>Thanks and Regards,
>Arun R Murthy
>-------------------
diff mbox series

Patch

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8b48a1974da3..3984cfa00cbf 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -274,6 +274,38 @@  struct drm_crtc_state {
 	 */
 	struct drm_property_blob *gamma_lut;
 
+	/**
+	 * @histogram_enable:
+	 *
+	 * This will be set if histogram is enabled for the CRTC.
+	 */
+	bool histogram_enable;
+
+	/**
+	 * @histogram_data:
+	 *
+	 * This will hold the pointer to the struct drm_histogram.
+	 * The element data in drm_histogram will hold the pointer to the
+	 * histogram data generated by the hardware.
+	 */
+	struct drm_property_blob *histogram_data;
+
+	/**
+	 * @histogram_-iet:
+	 *
+	 * This will hold the pointer to the struct drm_histogram.
+	 * The element data in drm_histogram will hold the pointer to the
+	 * histogram image enhancement generated by the algorithm that is to
+	 * be fed back to the hardware.
+	 */
+	struct drm_property_blob *histogram_iet;
+	/**
+	 * @histogram_iet_updates:
+	 *
+	 * Convey that the image enhanced data has been updated by the user
+	 */
+	bool histogram_iet_updated;
+
 	/**
 	 * @target_vblank:
 	 *
@@ -1088,6 +1120,22 @@  struct drm_crtc {
 	 */
 	struct drm_property *scaling_filter_property;
 
+	/**
+	 * @histogram_enable_property: Optional CRTC property for enabling or
+	 * disabling global histogram.
+	 */
+	struct drm_property *histogram_enable_property;
+	/**
+	 * @histogram_data_proeprty: Optional CRTC property for getting the
+	 * histogram blob data.
+	 */
+	struct drm_property *histogram_data_property;
+	/**
+	 * @histogram_iet_proeprty: Optional CRTC property for writing the
+	 * image enhanced blob data
+	 */
+	struct drm_property *histogram_iet_property;
+
 	/**
 	 * @state:
 	 *
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index c082810c08a8..da4396f57ed1 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -1355,6 +1355,17 @@  struct drm_mode_closefb {
 	__u32 pad;
 };
 
+/**
+ * struct drm_histogram
+ * data_ptr: pointer to the array fo u32 data. This data can be histogram
+ * raw data or image enhanced data
+ * nr_elements: number of elements pointed by the data @data_ptr
+ */
+struct drm_histogram {
+	__u64 data_ptr;
+	__u32 nr_elements;
+};
+
 #if defined(__cplusplus)
 }
 #endif