[PATCH 00/10] Improve crc-core driver interface

[PATCH 00/10] Improve crc-core driver interface

[Mahesh Kumar]

This series improves crc-core <-> driver interface.
This series adds following functionality in the crc-core
 - Now control node will print all the available sources if
   implemented by driver along with current source.
 - Setting of sorce will fail if provided source is not supported
 - cleanup of crtc_crc_open function first allocate memory before
   enabling CRC generation
 - Don't block open() call instead wait in crc read call.

Following IGT will fail due to crc-core <-> driver interface change
igt@kms_pipe_crc_basic@bad-source <now setting bad-source itself will fail>
ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X 
ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X-frame-sequence
In nonblocking crc tests we'll get lesser crc's due to skipping crc

AMD/Rockchip/rcar code path is not validated and need inputs

Changes:
 - Add dri-devel in Cc
Changes rev2:
 - now get_crc_sources returns a constant pointer to an array of
   source list and crc-core does the verification
Changes rev3:
 - reorg patches to push non r-b patches to the last
 - add r-b tag

Cc: dri-devel@lists.freedesktop.org

Mahesh Kumar (10):
  drm: crc: Introduce verify_crc_source callback
  drm: crc: Introduce get_crc_sources callback
  drm/rockchip/crc: Implement verify_crc_source callback
  drm/amdgpu_dm/crc: Implement verify_crc_source callback
  drm/rcar-du/crc: Implement verify_crc_source callback
  drm/i915/crc: implement verify_crc_source callback
  drm/i915/crc: implement get_crc_sources callback
  drm/crc: Cleanup crtc_crc_open function
  Revert "drm: crc: Wait for a frame before returning from open()"
  drm: crc: Introduce pre_crc_read function

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |   1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   7 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  20 +++-
 drivers/gpu/drm/drm_debugfs_crc.c                  |  79 ++++++++------
 drivers/gpu/drm/i915/intel_display.c               |   2 +
 drivers/gpu/drm/i915/intel_drv.h                   |   9 +-
 drivers/gpu/drm/i915/intel_pipe_crc.c              | 119 ++++++++++++++++++++-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  45 +++++++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  26 ++++-
 include/drm/drm_crtc.h                             |  48 ++++++++-
 10 files changed, 305 insertions(+), 51 deletions(-)

-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/10] drm: crc: Introduce verify_crc_source callback

[Mahesh Kumar]

This patch adds a new callback function "verify_crc_source" which will
be used during setting the crc source in control node and while opening
data node for crc reading. This will help in avoiding setting of wrong
string for source.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++
 include/drm/drm_crtc.h            | 15 +++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 9f8312137cad..c6a725b79ac6 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	struct drm_crtc *crtc = m->private;
 	struct drm_crtc_crc *crc = &crtc->crc;
 	char *source;
+	size_t values_cnt;
+	int ret = 0;
 
 	if (len == 0)
 		return 0;
@@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	if (source[len] == '\n')
 		source[len] = '\0';
 
+	if (crtc->funcs->verify_crc_source) {
+		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
+		if (ret)
+			return ret;
+	}
+
 	spin_lock_irq(&crc->lock);
 
 	if (crc->opened) {
@@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 			return ret;
 	}
 
+	if (crtc->funcs->verify_crc_source) {
+		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
+						     &values_cnt);
+		if (ret)
+			return ret;
+	}
+
 	spin_lock_irq(&crc->lock);
 	if (!crc->opened)
 		crc->opened = true;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 23eddbccab10..1a6dcbf91744 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -661,6 +661,21 @@ struct drm_crtc_funcs {
 	 */
 	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
 			      size_t *values_cnt);
+	/**
+	 * @verify_crc_source:
+	 *
+	 * verifies the source of CRC checksums of frames before setting the
+	 * source for CRC and during crc open.
+	 *
+	 * This callback is optional if the driver does not support any CRC
+	 * generation functionality.
+	 *
+	 * RETURNS:
+	 *
+	 * 0 on success or a negative error code on failure.
+	 */
+	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
+				 size_t *values_cnt);
 
 	/**
 	 * @atomic_print_state:
-- 
2.16.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/10] drm: crc: Introduce verify_crc_source callback

[Laurent Pinchart]

Hi Mahesh,

Thank you for the patch.

On Monday, 2 July 2018 14:07:20 EEST Mahesh Kumar wrote:
> This patch adds a new callback function "verify_crc_source" which will
> be used during setting the crc source in control node and while opening
> data node for crc reading. This will help in avoiding setting of wrong
> string for source.

Why do you need to verify the source in the open() operation ? Isn't it enough 
to verify it in the write() handler ? The answer to this question might lie in 
patch 08/10, in which case I'd add the .verify_crc_source() call in open() in 
that patch, not here.

> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++
>  include/drm/drm_crtc.h            | 15 +++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> b/drivers/gpu/drm/drm_debugfs_crc.c index 9f8312137cad..c6a725b79ac6 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const
> char __user *ubuf, struct drm_crtc *crtc = m->private;
>  	struct drm_crtc_crc *crc = &crtc->crc;
>  	char *source;
> +	size_t values_cnt;
> +	int ret = 0;
> 
>  	if (len == 0)
>  		return 0;
> @@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file,
> const char __user *ubuf, if (source[len] == '\n')
>  		source[len] = '\0';
> 
> +	if (crtc->funcs->verify_crc_source) {
> +		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	spin_lock_irq(&crc->lock);
> 
>  	if (crc->opened) {
> @@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct
> file *filep) return ret;
>  	}
> 
> +	if (crtc->funcs->verify_crc_source) {
> +		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
> +						     &values_cnt);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	spin_lock_irq(&crc->lock);
>  	if (!crc->opened)
>  		crc->opened = true;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 23eddbccab10..1a6dcbf91744 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -661,6 +661,21 @@ struct drm_crtc_funcs {
>  	 */
>  	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
>  			      size_t *values_cnt);
> +	/**
> +	 * @verify_crc_source:
> +	 *
> +	 * verifies the source of CRC checksums of frames before setting the
> +	 * source for CRC and during crc open.
> +	 *
> +	 * This callback is optional if the driver does not support any CRC
> +	 * generation functionality.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success or a negative error code on failure.
> +	 */
> +	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
> +				 size_t *values_cnt);
> 
>  	/**
>  	 * @atomic_print_state:


-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/10] drm: crc: Introduce verify_crc_source callback

[Kumar, Mahesh]

Hi,

Thanks for the review,
On 7/10/2018 4:56 PM, Laurent Pinchart wrote:
> Hi Mahesh,
>
> Thank you for the patch.
>
> On Monday, 2 July 2018 14:07:20 EEST Mahesh Kumar wrote:
>> This patch adds a new callback function "verify_crc_source" which will
>> be used during setting the crc source in control node and while opening
>> data node for crc reading. This will help in avoiding setting of wrong
>> string for source.
> Why do you need to verify the source in the open() operation ? Isn't it enough
> to verify it in the write() handler ? The answer to this question might lie in
> patch 08/10, in which case I'd add the .verify_crc_source() call in open() in
> that patch, not here.
Yes, final goal is achieved by patch 08/10. But if crc_read is called 
before setting the source, it may have wrong source set in that case I 
wanted to return early at least for the drivers implemented 
verify_crc_source. If you still think otherwise I will modify 
accordingly in next series.

-Mahesh

>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++
>>   include/drm/drm_crtc.h            | 15 +++++++++++++++
>>   2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
>> b/drivers/gpu/drm/drm_debugfs_crc.c index 9f8312137cad..c6a725b79ac6 100644
>> --- a/drivers/gpu/drm/drm_debugfs_crc.c
>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
>> @@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const
>> char __user *ubuf, struct drm_crtc *crtc = m->private;
>>   	struct drm_crtc_crc *crc = &crtc->crc;
>>   	char *source;
>> +	size_t values_cnt;
>> +	int ret = 0;
>>
>>   	if (len == 0)
>>   		return 0;
>> @@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file,
>> const char __user *ubuf, if (source[len] == '\n')
>>   		source[len] = '\0';
>>
>> +	if (crtc->funcs->verify_crc_source) {
>> +		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>   	spin_lock_irq(&crc->lock);
>>
>>   	if (crc->opened) {
>> @@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct
>> file *filep) return ret;
>>   	}
>>
>> +	if (crtc->funcs->verify_crc_source) {
>> +		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
>> +						     &values_cnt);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>   	spin_lock_irq(&crc->lock);
>>   	if (!crc->opened)
>>   		crc->opened = true;
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 23eddbccab10..1a6dcbf91744 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -661,6 +661,21 @@ struct drm_crtc_funcs {
>>   	 */
>>   	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
>>   			      size_t *values_cnt);
>> +	/**
>> +	 * @verify_crc_source:
>> +	 *
>> +	 * verifies the source of CRC checksums of frames before setting the
>> +	 * source for CRC and during crc open.
>> +	 *
>> +	 * This callback is optional if the driver does not support any CRC
>> +	 * generation functionality.
>> +	 *
>> +	 * RETURNS:
>> +	 *
>> +	 * 0 on success or a negative error code on failure.
>> +	 */
>> +	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
>> +				 size_t *values_cnt);
>>
>>   	/**
>>   	 * @atomic_print_state:
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/10] drm: crc: Introduce verify_crc_source callback

[Laurent Pinchart]

Hi Mahesh,

On Tuesday, 10 July 2018 14:54:11 EEST Kumar, Mahesh wrote:
> On 7/10/2018 4:56 PM, Laurent Pinchart wrote:
> > On Monday, 2 July 2018 14:07:20 EEST Mahesh Kumar wrote:
> >> This patch adds a new callback function "verify_crc_source" which will
> >> be used during setting the crc source in control node and while opening
> >> data node for crc reading. This will help in avoiding setting of wrong
> >> string for source.
> > 
> > Why do you need to verify the source in the open() operation ? Isn't it
> > enough to verify it in the write() handler ? The answer to this question
> > might lie in patch 08/10, in which case I'd add the .verify_crc_source()
> > call in open() in that patch, not here.
> 
> Yes, final goal is achieved by patch 08/10. But if crc_read is called
> before setting the source, it may have wrong source set in that case I
> wanted to return early at least for the drivers implemented
> verify_crc_source. If you still think otherwise I will modify
> accordingly in next series.

I don't disagree with you, but I don't think this issue is new. As I got a bit 
confused as to why the call is needed in open() (there's no explanation in 
this patch), I think fixing the problem in 08/10 would be better.

> >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >> 
> >>   drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++
> >>   include/drm/drm_crtc.h            | 15 +++++++++++++++
> >>   2 files changed, 30 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> >> b/drivers/gpu/drm/drm_debugfs_crc.c index 9f8312137cad..c6a725b79ac6
> >> 100644
> >> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> >> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> >> @@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file,
> >> const
> >> char __user *ubuf, struct drm_crtc *crtc = m->private;
> >> 
> >>   	struct drm_crtc_crc *crc = &crtc->crc;
> >>   	char *source;
> >> 
> >> +	size_t values_cnt;
> >> +	int ret = 0;
> >> 
> >>   	if (len == 0)
> >>   	
> >>   		return 0;
> >> 
> >> @@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file,
> >> const char __user *ubuf, if (source[len] == '\n')
> >> 
> >>   		source[len] = '\0';
> >> 
> >> +	if (crtc->funcs->verify_crc_source) {
> >> +		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> 
> >>   	spin_lock_irq(&crc->lock);
> >>   	
> >>   	if (crc->opened) {
> >> 
> >> @@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct
> >> file *filep) return ret;
> >> 
> >>   	}
> >> 
> >> +	if (crtc->funcs->verify_crc_source) {
> >> +		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
> >> +						     &values_cnt);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> 
> >>   	spin_lock_irq(&crc->lock);
> >>   	if (!crc->opened)
> >>   	
> >>   		crc->opened = true;
> >> 
> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >> index 23eddbccab10..1a6dcbf91744 100644
> >> --- a/include/drm/drm_crtc.h
> >> +++ b/include/drm/drm_crtc.h
> >> @@ -661,6 +661,21 @@ struct drm_crtc_funcs {
> >> 
> >>   	 */
> >>   	
> >>   	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
> >>   	
> >>   			      size_t *values_cnt);
> >> 
> >> +	/**
> >> +	 * @verify_crc_source:
> >> +	 *
> >> +	 * verifies the source of CRC checksums of frames before setting the
> >> +	 * source for CRC and during crc open.
> >> +	 *
> >> +	 * This callback is optional if the driver does not support any CRC
> >> +	 * generation functionality.
> >> +	 *
> >> +	 * RETURNS:
> >> +	 *
> >> +	 * 0 on success or a negative error code on failure.
> >> +	 */
> >> +	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
> >> +				 size_t *values_cnt);
> >> 
> >>   	/**
> >>   	
> >>   	 * @atomic_print_state:


-- 
Regards,

Laurent Pinchart



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/10] drm: crc: Introduce verify_crc_source callback

[Kumar, Mahesh]

Hi,


On 7/10/2018 5:40 PM, Laurent Pinchart wrote:
> Hi Mahesh,
>
> On Tuesday, 10 July 2018 14:54:11 EEST Kumar, Mahesh wrote:
>> On 7/10/2018 4:56 PM, Laurent Pinchart wrote:
>>> On Monday, 2 July 2018 14:07:20 EEST Mahesh Kumar wrote:
>>>> This patch adds a new callback function "verify_crc_source" which will
>>>> be used during setting the crc source in control node and while opening
>>>> data node for crc reading. This will help in avoiding setting of wrong
>>>> string for source.
>>> Why do you need to verify the source in the open() operation ? Isn't it
>>> enough to verify it in the write() handler ? The answer to this question
>>> might lie in patch 08/10, in which case I'd add the .verify_crc_source()
>>> call in open() in that patch, not here.
>> Yes, final goal is achieved by patch 08/10. But if crc_read is called
>> before setting the source, it may have wrong source set in that case I
>> wanted to return early at least for the drivers implemented
>> verify_crc_source. If you still think otherwise I will modify
>> accordingly in next series.
> I don't disagree with you, but I don't think this issue is new. As I got a bit
> confused as to why the call is needed in open() (there's no explanation in
> this patch), I think fixing the problem in 08/10 would be better.
ok sure :)
-Mahesh
>
>>>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>
>>>>    drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++
>>>>    include/drm/drm_crtc.h            | 15 +++++++++++++++
>>>>    2 files changed, 30 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
>>>> b/drivers/gpu/drm/drm_debugfs_crc.c index 9f8312137cad..c6a725b79ac6
>>>> 100644
>>>> --- a/drivers/gpu/drm/drm_debugfs_crc.c
>>>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
>>>> @@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file,
>>>> const
>>>> char __user *ubuf, struct drm_crtc *crtc = m->private;
>>>>
>>>>    	struct drm_crtc_crc *crc = &crtc->crc;
>>>>    	char *source;
>>>>
>>>> +	size_t values_cnt;
>>>> +	int ret = 0;
>>>>
>>>>    	if (len == 0)
>>>>    	
>>>>    		return 0;
>>>>
>>>> @@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file,
>>>> const char __user *ubuf, if (source[len] == '\n')
>>>>
>>>>    		source[len] = '\0';
>>>>
>>>> +	if (crtc->funcs->verify_crc_source) {
>>>> +		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>>
>>>>    	spin_lock_irq(&crc->lock);
>>>>    	
>>>>    	if (crc->opened) {
>>>>
>>>> @@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct
>>>> file *filep) return ret;
>>>>
>>>>    	}
>>>>
>>>> +	if (crtc->funcs->verify_crc_source) {
>>>> +		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
>>>> +						     &values_cnt);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>>
>>>>    	spin_lock_irq(&crc->lock);
>>>>    	if (!crc->opened)
>>>>    	
>>>>    		crc->opened = true;
>>>>
>>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>>>> index 23eddbccab10..1a6dcbf91744 100644
>>>> --- a/include/drm/drm_crtc.h
>>>> +++ b/include/drm/drm_crtc.h
>>>> @@ -661,6 +661,21 @@ struct drm_crtc_funcs {
>>>>
>>>>    	 */
>>>>    	
>>>>    	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
>>>>    	
>>>>    			      size_t *values_cnt);
>>>>
>>>> +	/**
>>>> +	 * @verify_crc_source:
>>>> +	 *
>>>> +	 * verifies the source of CRC checksums of frames before setting the
>>>> +	 * source for CRC and during crc open.
>>>> +	 *
>>>> +	 * This callback is optional if the driver does not support any CRC
>>>> +	 * generation functionality.
>>>> +	 *
>>>> +	 * RETURNS:
>>>> +	 *
>>>> +	 * 0 on success or a negative error code on failure.
>>>> +	 */
>>>> +	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
>>>> +				 size_t *values_cnt);
>>>>
>>>>    	/**
>>>>    	
>>>>    	 * @atomic_print_state:
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 02/10] drm: crc: Introduce get_crc_sources callback

[Mahesh Kumar]

This patch introduce a callback function "get_crc_sources" which
will be called during read of control node. It is an optional
callback function and if driver implements this callback, driver
should print list of available CRC sources in seq_file privided
as an input to the callback.

Changes Since V1: (Daniel)
 - return const pointer to an array of crc sources list
 - do validation of sources in CRC-core

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c | 20 +++++++++++++++++++-
 include/drm/drm_crtc.h            | 16 ++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index c6a725b79ac6..f4d76528d24c 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -67,9 +67,27 @@
 static int crc_control_show(struct seq_file *m, void *data)
 {
 	struct drm_crtc *crtc = m->private;
+	size_t count;
+
+	if (crtc->funcs->get_crc_sources) {
+		const char *const *sources = crtc->funcs->get_crc_sources(crtc,
+									&count);
+		size_t values_cnt;
+		int i;
+
+		if (count <= 0 || !sources)
+			goto out;
+
+		seq_puts(m, "[");
+		for (i = 0; i < count; i++)
+			if (!crtc->funcs->verify_crc_source(crtc, sources[i],
+							    &values_cnt))
+				seq_printf(m, "%s ", sources[i]);
+		seq_puts(m, "] ");
+	}
 
+out:
 	seq_printf(m, "%s\n", crtc->crc.source);
-
 	return 0;
 }
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 1a6dcbf91744..ffaec138aeee 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -676,6 +676,22 @@ struct drm_crtc_funcs {
 	 */
 	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
 				 size_t *values_cnt);
+	/**
+	 * @get_crc_sources:
+	 *
+	 * Driver callback for getting a list of all the available sources for
+	 * CRC generation.
+	 *
+	 * This callback is optional if the driver does not support exporting of
+	 * possible CRC sources list. CRC-core does the verification of sources.
+	 *
+	 * RETURNS:
+	 *
+	 * a constant character pointer to the list of all the available CRC
+	 * sources
+	 */
+	const char *const *(*get_crc_sources)(struct drm_crtc *crtc,
+					      size_t *count);
 
 	/**
 	 * @atomic_print_state:
-- 
2.16.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/10] drm: crc: Introduce get_crc_sources callback

[Laurent Pinchart]

Hi Mahesh,

Thank you for the patch.

On Monday, 2 July 2018 14:07:21 EEST Mahesh Kumar wrote:
> This patch introduce a callback function "get_crc_sources" which
> will be called during read of control node. It is an optional
> callback function and if driver implements this callback, driver
> should print list of available CRC sources in seq_file privided
> as an input to the callback.

The commit message seems to be outdated, the callback doesn't take a seq_file 
anymore.

> Changes Since V1: (Daniel)
>  - return const pointer to an array of crc sources list
>  - do validation of sources in CRC-core
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 20 +++++++++++++++++++-
>  include/drm/drm_crtc.h            | 16 ++++++++++++++++
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> b/drivers/gpu/drm/drm_debugfs_crc.c index c6a725b79ac6..f4d76528d24c 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -67,9 +67,27 @@
>  static int crc_control_show(struct seq_file *m, void *data)
>  {
>  	struct drm_crtc *crtc = m->private;
> +	size_t count;

Count it only used within the if () {} block, you can declare it there.

> +
> +	if (crtc->funcs->get_crc_sources) {
> +		const char *const *sources = crtc->funcs->get_crc_sources(crtc,
> +									&count);
> +		size_t values_cnt;
> +		int i;

I only takes positive values, it can be an unsigned int.

> +
> +		if (count <= 0 || !sources)

count is a size_t, it can't be negative.

The .get_crc_sources() documentation doesn't clearly specify whether sources 
should always be NULL when count is zero. I advise updating the documentation, 
and possibly updating this check accordingly.

> +			goto out;
> +
> +		seq_puts(m, "[");
> +		for (i = 0; i < count; i++)
> +			if (!crtc->funcs->verify_crc_source(crtc, sources[i],
> +							    &values_cnt))

I assume that you verify sources one by one here to avoid having to create a 
list of sources dynamically in the .get_crc_sources() callback ? If so, I 
think the .get_crc_sources() callback should document that.

You should also document that .verify_crc_source() is required when 
get_crc_sources() is provided.

> +				seq_printf(m, "%s ", sources[i]);
> +		seq_puts(m, "] ");

This assumes that source names can't include a space. Isn't that too 
restrictive ? Shouldn't a different separator be used ? How about one source 
name per line ?

Additionally, shouldn't the active source be marked ?

> +	}
> 
> +out:
>  	seq_printf(m, "%s\n", crtc->crc.source);
> -
>  	return 0;
>  }
> 
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 1a6dcbf91744..ffaec138aeee 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -676,6 +676,22 @@ struct drm_crtc_funcs {
>  	 */
>  	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
>  				 size_t *values_cnt);
> +	/**
> +	 * @get_crc_sources:
> +	 *
> +	 * Driver callback for getting a list of all the available sources for
> +	 * CRC generation.
> +	 *
> +	 * This callback is optional if the driver does not support exporting of
> +	 * possible CRC sources list. CRC-core does the verification of sources.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * a constant character pointer to the list of all the available CRC
> +	 * sources
> +	 */
> +	const char *const *(*get_crc_sources)(struct drm_crtc *crtc,
> +					      size_t *count);
> 
>  	/**
>  	 * @atomic_print_state:

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/10] drm: crc: Introduce get_crc_sources callback

[Kumar, Mahesh]

Hi,

thanks for the review.
On 7/10/2018 4:52 PM, Laurent Pinchart wrote:
> Hi Mahesh,
>
> Thank you for the patch.
>
> On Monday, 2 July 2018 14:07:21 EEST Mahesh Kumar wrote:
>> This patch introduce a callback function "get_crc_sources" which
>> will be called during read of control node. It is an optional
>> callback function and if driver implements this callback, driver
>> should print list of available CRC sources in seq_file privided
>> as an input to the callback.
> The commit message seems to be outdated, the callback doesn't take a seq_file
> anymore.
ops, will update.
>
>> Changes Since V1: (Daniel)
>>   - return const pointer to an array of crc sources list
>>   - do validation of sources in CRC-core
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   drivers/gpu/drm/drm_debugfs_crc.c | 20 +++++++++++++++++++-
>>   include/drm/drm_crtc.h            | 16 ++++++++++++++++
>>   2 files changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
>> b/drivers/gpu/drm/drm_debugfs_crc.c index c6a725b79ac6..f4d76528d24c 100644
>> --- a/drivers/gpu/drm/drm_debugfs_crc.c
>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
>> @@ -67,9 +67,27 @@
>>   static int crc_control_show(struct seq_file *m, void *data)
>>   {
>>   	struct drm_crtc *crtc = m->private;
>> +	size_t count;
> Count it only used within the if () {} block, you can declare it there.
agree.
>
>> +
>> +	if (crtc->funcs->get_crc_sources) {
>> +		const char *const *sources = crtc->funcs->get_crc_sources(crtc,
>> +									&count);
>> +		size_t values_cnt;
>> +		int i;
> I only takes positive values, it can be an unsigned int.
ok
>
>> +
>> +		if (count <= 0 || !sources)
> count is a size_t, it can't be negative.
>
> The .get_crc_sources() documentation doesn't clearly specify whether sources
> should always be NULL when count is zero. I advise updating the documentation,
> and possibly updating this check accordingly.
ok will update.
>
>> +			goto out;
>> +
>> +		seq_puts(m, "[");
>> +		for (i = 0; i < count; i++)
>> +			if (!crtc->funcs->verify_crc_source(crtc, sources[i],
>> +							    &values_cnt))
> I assume that you verify sources one by one here to avoid having to create a
> list of sources dynamically in the .get_crc_sources() callback ? If so, I
> think the .get_crc_sources() callback should document that.
>
> You should also document that .verify_crc_source() is required when
> get_crc_sources() is provided.
ok sure.
>
>> +				seq_printf(m, "%s ", sources[i]);
>> +		seq_puts(m, "] ");
> This assumes that source names can't include a space. Isn't that too
> restrictive ? Shouldn't a different separator be used ? How about one source
> name per line ?
what about comma separated as I'm putting names inside square-brackets?
>
> Additionally, shouldn't the active source be marked ?
active source is again printed by the code in next few lines. output 
will be of following format.
[space separated list of valid sources] active_source

-Mahesh
>
>> +	}
>>
>> +out:
>>   	seq_printf(m, "%s\n", crtc->crc.source);
>> -
>>   	return 0;
>>   }
>>
>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>> index 1a6dcbf91744..ffaec138aeee 100644
>> --- a/include/drm/drm_crtc.h
>> +++ b/include/drm/drm_crtc.h
>> @@ -676,6 +676,22 @@ struct drm_crtc_funcs {
>>   	 */
>>   	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
>>   				 size_t *values_cnt);
>> +	/**
>> +	 * @get_crc_sources:
>> +	 *
>> +	 * Driver callback for getting a list of all the available sources for
>> +	 * CRC generation.
>> +	 *
>> +	 * This callback is optional if the driver does not support exporting of
>> +	 * possible CRC sources list. CRC-core does the verification of sources.
>> +	 *
>> +	 * RETURNS:
>> +	 *
>> +	 * a constant character pointer to the list of all the available CRC
>> +	 * sources
>> +	 */
>> +	const char *const *(*get_crc_sources)(struct drm_crtc *crtc,
>> +					      size_t *count);
>>
>>   	/**
>>   	 * @atomic_print_state:

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/10] drm: crc: Introduce get_crc_sources callback

[Laurent Pinchart]

Hi Mahesh,

On Tuesday, 10 July 2018 15:01:38 EEST Kumar, Mahesh wrote:
> On 7/10/2018 4:52 PM, Laurent Pinchart wrote:
> > Hi Mahesh,
> > On Monday, 2 July 2018 14:07:21 EEST Mahesh Kumar wrote:
> >> This patch introduce a callback function "get_crc_sources" which
> >> will be called during read of control node. It is an optional
> >> callback function and if driver implements this callback, driver
> >> should print list of available CRC sources in seq_file privided
> >> as an input to the callback.
> > 
> > The commit message seems to be outdated, the callback doesn't take a
> > seq_file anymore.
> 
> ops, will update.
> 
> >> Changes Since V1: (Daniel)
> >> 
> >>   - return const pointer to an array of crc sources list
> >>   - do validation of sources in CRC-core
> >> 
> >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >> 
> >>   drivers/gpu/drm/drm_debugfs_crc.c | 20 +++++++++++++++++++-
> >>   include/drm/drm_crtc.h            | 16 ++++++++++++++++
> >>   2 files changed, 35 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> >> b/drivers/gpu/drm/drm_debugfs_crc.c index c6a725b79ac6..f4d76528d24c
> >> 100644
> >> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> >> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> >> @@ -67,9 +67,27 @@
> >> 
> >>   static int crc_control_show(struct seq_file *m, void *data)
> >>   {
> >>   
> >>   	struct drm_crtc *crtc = m->private;
> >> 
> >> +	size_t count;
> > 
> > Count it only used within the if () {} block, you can declare it there.
> 
> agree.
> 
> >> +
> >> +	if (crtc->funcs->get_crc_sources) {
> >> +		const char *const *sources = crtc->funcs->get_crc_sources(crtc,
> >> +									&count);
> >> +		size_t values_cnt;
> >> +		int i;
> > 
> > I only takes positive values, it can be an unsigned int.
> 
> ok
> 
> >> +
> >> +		if (count <= 0 || !sources)
> > 
> > count is a size_t, it can't be negative.
> > 
> > The .get_crc_sources() documentation doesn't clearly specify whether
> > sources should always be NULL when count is zero. I advise updating the
> > documentation, and possibly updating this check accordingly.
> 
> ok will update.
> 
> >> +			goto out;
> >> +
> >> +		seq_puts(m, "[");
> >> +		for (i = 0; i < count; i++)
> >> +			if (!crtc->funcs->verify_crc_source(crtc, sources[i],
> >> +							    &values_cnt))
> > 
> > I assume that you verify sources one by one here to avoid having to create
> > a list of sources dynamically in the .get_crc_sources() callback ? If so,
> > I think the .get_crc_sources() callback should document that.
> > 
> > You should also document that .verify_crc_source() is required when
> > get_crc_sources() is provided.
> 
> ok sure.
> 
> >> +				seq_printf(m, "%s ", sources[i]);
> >> +		seq_puts(m, "] ");
> > 
> > This assumes that source names can't include a space. Isn't that too
> > restrictive ? Shouldn't a different separator be used ? How about one
> > source name per line ?
> 
> what about comma separated as I'm putting names inside square-brackets?
> 
> > Additionally, shouldn't the active source be marked ?
> 
> active source is again printed by the code in next few lines. output
> will be of following format.
> [space separated list of valid sources] active_source

I had missed that, my bad.

The proposed format seems a bit hackish to me, in the sense that it forbids 
both spaces and brackets in source names. One source per line would fix both 
and be easy to parse. We would then need to mark the active source, which 
could be done by adding a marker to the corresponding line (maybe a * at the 
end of the line ?).

> >> +	}
> >> 
> >> +out:
> >>   	seq_printf(m, "%s\n", crtc->crc.source);
> >> -
> >>   	return 0;
> >>   }

[snip]

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/10] drm: crc: Introduce get_crc_sources callback

[Kumar, Mahesh]

Hi,


On 7/10/2018 5:39 PM, Laurent Pinchart wrote:
> Hi Mahesh,
>
> On Tuesday, 10 July 2018 15:01:38 EEST Kumar, Mahesh wrote:
>> On 7/10/2018 4:52 PM, Laurent Pinchart wrote:
>>> Hi Mahesh,
>>> On Monday, 2 July 2018 14:07:21 EEST Mahesh Kumar wrote:
>>>> This patch introduce a callback function "get_crc_sources" which
>>>> will be called during read of control node. It is an optional
>>>> callback function and if driver implements this callback, driver
>>>> should print list of available CRC sources in seq_file privided
>>>> as an input to the callback.
>>> The commit message seems to be outdated, the callback doesn't take a
>>> seq_file anymore.
>> ops, will update.
>>
>>>> Changes Since V1: (Daniel)
>>>>
>>>>    - return const pointer to an array of crc sources list
>>>>    - do validation of sources in CRC-core
>>>>
>>>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>
>>>>    drivers/gpu/drm/drm_debugfs_crc.c | 20 +++++++++++++++++++-
>>>>    include/drm/drm_crtc.h            | 16 ++++++++++++++++
>>>>    2 files changed, 35 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
>>>> b/drivers/gpu/drm/drm_debugfs_crc.c index c6a725b79ac6..f4d76528d24c
>>>> 100644
>>>> --- a/drivers/gpu/drm/drm_debugfs_crc.c
>>>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
>>>> @@ -67,9 +67,27 @@
>>>>
>>>>    static int crc_control_show(struct seq_file *m, void *data)
>>>>    {
>>>>    
>>>>    	struct drm_crtc *crtc = m->private;
>>>>
>>>> +	size_t count;
>>> Count it only used within the if () {} block, you can declare it there.
>> agree.
>>
>>>> +
>>>> +	if (crtc->funcs->get_crc_sources) {
>>>> +		const char *const *sources = crtc->funcs->get_crc_sources(crtc,
>>>> +									&count);
>>>> +		size_t values_cnt;
>>>> +		int i;
>>> I only takes positive values, it can be an unsigned int.
>> ok
>>
>>>> +
>>>> +		if (count <= 0 || !sources)
>>> count is a size_t, it can't be negative.
>>>
>>> The .get_crc_sources() documentation doesn't clearly specify whether
>>> sources should always be NULL when count is zero. I advise updating the
>>> documentation, and possibly updating this check accordingly.
>> ok will update.
>>
>>>> +			goto out;
>>>> +
>>>> +		seq_puts(m, "[");
>>>> +		for (i = 0; i < count; i++)
>>>> +			if (!crtc->funcs->verify_crc_source(crtc, sources[i],
>>>> +							    &values_cnt))
>>> I assume that you verify sources one by one here to avoid having to create
>>> a list of sources dynamically in the .get_crc_sources() callback ? If so,
>>> I think the .get_crc_sources() callback should document that.
>>>
>>> You should also document that .verify_crc_source() is required when
>>> get_crc_sources() is provided.
>> ok sure.
>>
>>>> +				seq_printf(m, "%s ", sources[i]);
>>>> +		seq_puts(m, "] ");
>>> This assumes that source names can't include a space. Isn't that too
>>> restrictive ? Shouldn't a different separator be used ? How about one
>>> source name per line ?
>> what about comma separated as I'm putting names inside square-brackets?
>>
>>> Additionally, shouldn't the active source be marked ?
>> active source is again printed by the code in next few lines. output
>> will be of following format.
>> [space separated list of valid sources] active_source
> I had missed that, my bad.
>
> The proposed format seems a bit hackish to me, in the sense that it forbids
> both spaces and brackets in source names. One source per line would fix both
> and be easy to parse. We would then need to mark the active source, which
> could be done by adding a marker to the corresponding line (maybe a * at the
> end of the line ?).
sounds good, will do that.
-Mahesh
>
>>>> +	}
>>>>
>>>> +out:
>>>>    	seq_printf(m, "%s\n", crtc->crc.source);
>>>> -
>>>>    	return 0;
>>>>    }
> [snip]
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/10] drm/rockchip/crc: Implement verify_crc_source callback

[Mahesh Kumar]

This patch implements "verify_crc_source" callback function for
rockchip drm driver.

Changes since V1:
 - simplify the verification (Jani N)

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index effecbed2d11..77e91b15ddb4 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1138,12 +1138,31 @@ static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
 
 	return ret;
 }
+
+static int
+vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
+			   size_t *values_cnt)
+{
+	if (source_name && strcmp(source_name, "auto") != 0)
+		return -EINVAL;
+
+	*values_cnt = 3;
+	return 0;
+}
+
 #else
 static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
 				   const char *source_name, size_t *values_cnt)
 {
 	return -ENODEV;
 }
+
+static int
+vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
+			   size_t *values_cnt)
+{
+	return -ENODEV;
+}
 #endif
 
 static const struct drm_crtc_funcs vop_crtc_funcs = {
@@ -1156,6 +1175,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
 	.enable_vblank = vop_crtc_enable_vblank,
 	.disable_vblank = vop_crtc_disable_vblank,
 	.set_crc_source = vop_crtc_set_crc_source,
+	.verify_crc_source = vop_crtc_verify_crc_source,
 };
 
 static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/10] drm/rockchip/crc: Implement verify_crc_source callback

[Maarten Lankhorst]

Op 02-07-18 om 13:07 schreef Mahesh Kumar:
> This patch implements "verify_crc_source" callback function for
> rockchip drm driver.
>
> Changes since V1:
>  - simplify the verification (Jani N)
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index effecbed2d11..77e91b15ddb4 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1138,12 +1138,31 @@ static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
>  
>  	return ret;
>  }
> +
> +static int
> +vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> +			   size_t *values_cnt)
> +{
> +	if (source_name && strcmp(source_name, "auto") != 0)
> +		return -EINVAL;
> +
> +	*values_cnt = 3;
> +	return 0;
> +}
> +
>  #else
>  static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
>  				   const char *source_name, size_t *values_cnt)
>  {
>  	return -ENODEV;
>  }
> +
> +static int
> +vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> +			   size_t *values_cnt)
> +{
> +	return -ENODEV;
> +}
>  #endif
>  
>  static const struct drm_crtc_funcs vop_crtc_funcs = {
> @@ -1156,6 +1175,7 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
>  	.enable_vblank = vop_crtc_enable_vblank,
>  	.disable_vblank = vop_crtc_disable_vblank,
>  	.set_crc_source = vop_crtc_set_crc_source,
> +	.verify_crc_source = vop_crtc_verify_crc_source,
>  };
>  
>  static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)

Ack for merging this and 8/10 through drm-misc?

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/10] drm/rockchip/crc: Implement verify_crc_source callback

[Heiko Stübner]

Hi Maarten,

Am Dienstag, 3. Juli 2018, 12:16:41 CEST schrieb Maarten Lankhorst:
> Op 02-07-18 om 13:07 schreef Mahesh Kumar:
> > This patch implements "verify_crc_source" callback function for
> > rockchip drm driver.
> > 
> > Changes since V1:
> >  - simplify the verification (Jani N)
> > 
> > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> > Cc: dri-devel@lists.freedesktop.org
> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

> Ack for merging this and 8/10 through drm-misc?

Sure, looking over the series on dri-devel briefly, this looks of
course sane, so
Acked-by: Heiko Stuebner <heiko@sntech.de>


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/10] drm/amdgpu_dm/crc: Implement verify_crc_source callback

[Mahesh Kumar]

This patch implements "verify_crc_source" callback function for
AMD drm driver.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c     |  1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h     |  4 ++++
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 16 ++++++++++++++++
 3 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 66bd3cc3e387..dd97cbca0527 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2563,6 +2563,7 @@ static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
 	.atomic_duplicate_state = dm_crtc_duplicate_state,
 	.atomic_destroy_state = dm_crtc_destroy_state,
 	.set_crc_source = amdgpu_dm_crtc_set_crc_source,
+	.verify_crc_source = amdgpu_dm_crtc_verify_crc_source,
 	.enable_vblank = dm_enable_vblank,
 	.disable_vblank = dm_disable_vblank,
 };
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index a29dc35954c9..e43ed064dc46 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -260,9 +260,13 @@ amdgpu_dm_remove_sink_from_freesync_module(struct drm_connector *connector);
 #ifdef CONFIG_DEBUG_FS
 int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
 				  size_t *values_cnt);
+int amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc,
+				     const char *src_name,
+				     size_t *values_cnt);
 void amdgpu_dm_crtc_handle_crc_irq(struct drm_crtc *crtc);
 #else
 #define amdgpu_dm_crtc_set_crc_source NULL
+#define amdgpu_dm_crtc_verify_crc_source NULL
 #define amdgpu_dm_crtc_handle_crc_irq(x)
 #endif
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
index 52f2c01349e3..dfcca594d52a 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
@@ -46,6 +46,22 @@ static enum amdgpu_dm_pipe_crc_source dm_parse_crc_source(const char *source)
 	return AMDGPU_DM_PIPE_CRC_SOURCE_INVALID;
 }
 
+int
+amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
+				 size_t *values_cnt)
+{
+	enum amdgpu_dm_pipe_crc_source source = dm_parse_crc_source(src_name);
+
+	if (source < 0) {
+		DRM_DEBUG_DRIVER("Unknown CRC source %s for CRTC%d\n",
+				 src_name, crtc->index);
+		return -EINVAL;
+	}
+
+	*values_cnt = 3;
+	return 0;
+}
+
 int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
 			   size_t *values_cnt)
 {
-- 
2.16.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/10] drm/amdgpu_dm/crc: Implement verify_crc_source callback

[Maarten Lankhorst]

Op 02-07-18 om 13:07 schreef Mahesh Kumar:
> This patch implements "verify_crc_source" callback function for
> AMD drm driver.
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c     |  1 +
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h     |  4 ++++
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 16 ++++++++++++++++
>  3 files changed, 21 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 66bd3cc3e387..dd97cbca0527 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2563,6 +2563,7 @@ static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
>  	.atomic_duplicate_state = dm_crtc_duplicate_state,
>  	.atomic_destroy_state = dm_crtc_destroy_state,
>  	.set_crc_source = amdgpu_dm_crtc_set_crc_source,
> +	.verify_crc_source = amdgpu_dm_crtc_verify_crc_source,
>  	.enable_vblank = dm_enable_vblank,
>  	.disable_vblank = dm_disable_vblank,
>  };
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index a29dc35954c9..e43ed064dc46 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -260,9 +260,13 @@ amdgpu_dm_remove_sink_from_freesync_module(struct drm_connector *connector);
>  #ifdef CONFIG_DEBUG_FS
>  int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
>  				  size_t *values_cnt);
> +int amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc,
> +				     const char *src_name,
> +				     size_t *values_cnt);
>  void amdgpu_dm_crtc_handle_crc_irq(struct drm_crtc *crtc);
>  #else
>  #define amdgpu_dm_crtc_set_crc_source NULL
> +#define amdgpu_dm_crtc_verify_crc_source NULL
>  #define amdgpu_dm_crtc_handle_crc_irq(x)
>  #endif
>  
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> index 52f2c01349e3..dfcca594d52a 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> @@ -46,6 +46,22 @@ static enum amdgpu_dm_pipe_crc_source dm_parse_crc_source(const char *source)
>  	return AMDGPU_DM_PIPE_CRC_SOURCE_INVALID;
>  }
>  
> +int
> +amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
> +				 size_t *values_cnt)
> +{
> +	enum amdgpu_dm_pipe_crc_source source = dm_parse_crc_source(src_name);
> +
> +	if (source < 0) {
> +		DRM_DEBUG_DRIVER("Unknown CRC source %s for CRTC%d\n",
> +				 src_name, crtc->index);
> +		return -EINVAL;
> +	}
> +
> +	*values_cnt = 3;
> +	return 0;
> +}
> +
>  int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
>  			   size_t *values_cnt)
>  {

Ack for merging this and 8/10 through drm-misc?

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/10] drm/amdgpu_dm/crc: Implement verify_crc_source callback

[Leo Li]

On 2018-07-03 06:19 AM, Maarten Lankhorst wrote:
> Op 02-07-18 om 13:07 schreef Mahesh Kumar:
>> This patch implements "verify_crc_source" callback function for
>> AMD drm driver.
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Acked-by: Leo Li <sunpeng.li@amd.com>

>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c     |  1 +
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h     |  4 ++++
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c | 16 ++++++++++++++++
>>   3 files changed, 21 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 66bd3cc3e387..dd97cbca0527 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -2563,6 +2563,7 @@ static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = {
>>   	.atomic_duplicate_state = dm_crtc_duplicate_state,
>>   	.atomic_destroy_state = dm_crtc_destroy_state,
>>   	.set_crc_source = amdgpu_dm_crtc_set_crc_source,
>> +	.verify_crc_source = amdgpu_dm_crtc_verify_crc_source,
>>   	.enable_vblank = dm_enable_vblank,
>>   	.disable_vblank = dm_disable_vblank,
>>   };
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> index a29dc35954c9..e43ed064dc46 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> @@ -260,9 +260,13 @@ amdgpu_dm_remove_sink_from_freesync_module(struct drm_connector *connector);
>>   #ifdef CONFIG_DEBUG_FS
>>   int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
>>   				  size_t *values_cnt);
>> +int amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc,
>> +				     const char *src_name,
>> +				     size_t *values_cnt);
>>   void amdgpu_dm_crtc_handle_crc_irq(struct drm_crtc *crtc);
>>   #else
>>   #define amdgpu_dm_crtc_set_crc_source NULL
>> +#define amdgpu_dm_crtc_verify_crc_source NULL
>>   #define amdgpu_dm_crtc_handle_crc_irq(x)
>>   #endif
>>   
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
>> index 52f2c01349e3..dfcca594d52a 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
>> @@ -46,6 +46,22 @@ static enum amdgpu_dm_pipe_crc_source dm_parse_crc_source(const char *source)
>>   	return AMDGPU_DM_PIPE_CRC_SOURCE_INVALID;
>>   }
>>   
>> +int
>> +amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
>> +				 size_t *values_cnt)
>> +{
>> +	enum amdgpu_dm_pipe_crc_source source = dm_parse_crc_source(src_name);
>> +
>> +	if (source < 0) {
>> +		DRM_DEBUG_DRIVER("Unknown CRC source %s for CRTC%d\n",
>> +				 src_name, crtc->index);
>> +		return -EINVAL;
>> +	}
>> +
>> +	*values_cnt = 3;
>> +	return 0;
>> +}
>> +
>>   int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
>>   			   size_t *values_cnt)
>>   {
> 
> Ack for merging this and 8/10 through drm-misc?
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/10] drm/rcar-du/crc: Implement verify_crc_source callback

[Mahesh Kumar]

This patch implements "verify_crc_source" callback function for
rcar drm driver.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 40 ++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 15dc9caa128b..24eeaa7e14d7 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -756,6 +756,45 @@ static void rcar_du_crtc_disable_vblank(struct drm_crtc *crtc)
 	rcrtc->vblank_enable = false;
 }
 
+static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
+					  const char *source_name,
+					  size_t *values_cnt)
+{
+	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
+	unsigned int index = 0;
+	unsigned int i;
+	int ret;
+
+	/*
+	 * Parse the source name. Supported values are "plane%u" to compute the
+	 * CRC on an input plane (%u is the plane ID), and "auto" to compute the
+	 * CRC on the composer (VSP) output.
+	 */
+	if (!source_name || !strcmp(source_name, "auto")) {
+		goto out;
+	} else if (strstarts(source_name, "plane")) {
+		ret = kstrtouint(source_name + strlen("plane"), 10, &index);
+		if (ret < 0)
+			return ret;
+
+		for (i = 0; i < rcrtc->vsp->num_planes; ++i) {
+			if (index == rcrtc->vsp->planes[i].plane.base.id) {
+				index = i;
+				break;
+			}
+		}
+
+		if (i >= rcrtc->vsp->num_planes)
+			return -EINVAL;
+	} else {
+		return -EINVAL;
+	}
+
+out:
+	*values_cnt = 1;
+	return 0;
+}
+
 static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
 				       const char *source_name,
 				       size_t *values_cnt)
@@ -861,6 +900,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
 	.enable_vblank = rcar_du_crtc_enable_vblank,
 	.disable_vblank = rcar_du_crtc_disable_vblank,
 	.set_crc_source = rcar_du_crtc_set_crc_source,
+	.verify_crc_source = rcar_du_crtc_verify_crc_source,
 };
 
 /* -----------------------------------------------------------------------------
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/10] drm/rcar-du/crc: Implement verify_crc_source callback

[Maarten Lankhorst]

Op 02-07-18 om 13:07 schreef Mahesh Kumar:
> This patch implements "verify_crc_source" callback function for
> rcar drm driver.
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 40 ++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 15dc9caa128b..24eeaa7e14d7 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -756,6 +756,45 @@ static void rcar_du_crtc_disable_vblank(struct drm_crtc *crtc)
>  	rcrtc->vblank_enable = false;
>  }
>  
> +static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
> +					  const char *source_name,
> +					  size_t *values_cnt)
> +{
> +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +	unsigned int index = 0;
> +	unsigned int i;
> +	int ret;
> +
> +	/*
> +	 * Parse the source name. Supported values are "plane%u" to compute the
> +	 * CRC on an input plane (%u is the plane ID), and "auto" to compute the
> +	 * CRC on the composer (VSP) output.
> +	 */
> +	if (!source_name || !strcmp(source_name, "auto")) {
> +		goto out;
> +	} else if (strstarts(source_name, "plane")) {
> +		ret = kstrtouint(source_name + strlen("plane"), 10, &index);
> +		if (ret < 0)
> +			return ret;
> +
> +		for (i = 0; i < rcrtc->vsp->num_planes; ++i) {
> +			if (index == rcrtc->vsp->planes[i].plane.base.id) {
> +				index = i;
> +				break;
> +			}
> +		}
> +
> +		if (i >= rcrtc->vsp->num_planes)
> +			return -EINVAL;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +out:
> +	*values_cnt = 1;
> +	return 0;
> +}
> +
>  static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
>  				       const char *source_name,
>  				       size_t *values_cnt)
> @@ -861,6 +900,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
>  	.enable_vblank = rcar_du_crtc_enable_vblank,
>  	.disable_vblank = rcar_du_crtc_disable_vblank,
>  	.set_crc_source = rcar_du_crtc_set_crc_source,
> +	.verify_crc_source = rcar_du_crtc_verify_crc_source,
>  };
>  
>  /* -----------------------------------------------------------------------------

Ack for merging this and 8/10 through drm-misc?

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/10] drm/rcar-du/crc: Implement verify_crc_source callback

[Laurent Pinchart]

Hi Mahesh,

Thank you for the patch.

On Monday, 2 July 2018 14:07:24 EEST Mahesh Kumar wrote:
> This patch implements "verify_crc_source" callback function for
> rcar drm driver.
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 40 +++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 15dc9caa128b..24eeaa7e14d7
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -756,6 +756,45 @@ static void rcar_du_crtc_disable_vblank(struct drm_crtc
> *crtc) rcrtc->vblank_enable = false;
>  }
> 
> +static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
> +					  const char *source_name,
> +					  size_t *values_cnt)
> +{
> +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> +	unsigned int index = 0;
> +	unsigned int i;
> +	int ret;
> +
> +	/*
> +	 * Parse the source name. Supported values are "plane%u" to compute the
> +	 * CRC on an input plane (%u is the plane ID), and "auto" to compute the
> +	 * CRC on the composer (VSP) output.
> +	 */
> +	if (!source_name || !strcmp(source_name, "auto")) {
> +		goto out;
> +	} else if (strstarts(source_name, "plane")) {
> +		ret = kstrtouint(source_name + strlen("plane"), 10, &index);
> +		if (ret < 0)
> +			return ret;
> +
> +		for (i = 0; i < rcrtc->vsp->num_planes; ++i) {
> +			if (index == rcrtc->vsp->planes[i].plane.base.id) {
> +				index = i;
> +				break;
> +			}
> +		}
> +
> +		if (i >= rcrtc->vsp->num_planes)
> +			return -EINVAL;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +out:
> +	*values_cnt = 1;
> +	return 0;

This duplicates lots of code from the rcar_du_crtc_set_crc_source() function. 
Could you please extract it to a shared function ?

Could you please also implement support for the .get_crc_sources() operation ? 
I think doing so might show limitations in the current API, namely the fact 
that the list will need to be dynamically created for this driver.

> +}
> +
>  static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
>  				       const char *source_name,
>  				       size_t *values_cnt)
> @@ -861,6 +900,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
>  	.enable_vblank = rcar_du_crtc_enable_vblank,
>  	.disable_vblank = rcar_du_crtc_disable_vblank,
>  	.set_crc_source = rcar_du_crtc_set_crc_source,
> +	.verify_crc_source = rcar_du_crtc_verify_crc_source,
>  };
> 
>  /* ------------------------------------------------------------------------

-- 
Regards,

Laurent Pinchart



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/10] drm/rcar-du/crc: Implement verify_crc_source callback

[Kumar, Mahesh]

Hi,

thanks foe the review.

On 7/10/2018 5:07 PM, Laurent Pinchart wrote:
> Hi Mahesh,
>
> Thank you for the patch.
>
> On Monday, 2 July 2018 14:07:24 EEST Mahesh Kumar wrote:
>> This patch implements "verify_crc_source" callback function for
>> rcar drm driver.
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 40 +++++++++++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 15dc9caa128b..24eeaa7e14d7
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -756,6 +756,45 @@ static void rcar_du_crtc_disable_vblank(struct drm_crtc
>> *crtc) rcrtc->vblank_enable = false;
>>   }
>>
>> +static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
>> +					  const char *source_name,
>> +					  size_t *values_cnt)
>> +{
>> +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>> +	unsigned int index = 0;
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	/*
>> +	 * Parse the source name. Supported values are "plane%u" to compute the
>> +	 * CRC on an input plane (%u is the plane ID), and "auto" to compute the
>> +	 * CRC on the composer (VSP) output.
>> +	 */
>> +	if (!source_name || !strcmp(source_name, "auto")) {
>> +		goto out;
>> +	} else if (strstarts(source_name, "plane")) {
>> +		ret = kstrtouint(source_name + strlen("plane"), 10, &index);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		for (i = 0; i < rcrtc->vsp->num_planes; ++i) {
>> +			if (index == rcrtc->vsp->planes[i].plane.base.id) {
>> +				index = i;
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (i >= rcrtc->vsp->num_planes)
>> +			return -EINVAL;
>> +	} else {
>> +		return -EINVAL;
>> +	}
>> +
>> +out:
>> +	*values_cnt = 1;
>> +	return 0;
> This duplicates lots of code from the rcar_du_crtc_set_crc_source() function.
> Could you please extract it to a shared function ?
Agree, it duplicates the code but "index" is needed by set_crc_source call
anyway will create a wrapper to avoid duplication of code.
>
> Could you please also implement support for the .get_crc_sources() operation ?
> I think doing so might show limitations in the current API, namely the fact
> that the list will need to be dynamically created for this driver.
for that I think rcar_du_crtc_create function can build a dynamic list 
during initializing crtc functions, unless plane can be dynamically 
allocated to any CRTC. this is the place where I need input from maintainer.

-Mahesh
>
>> +}
>> +
>>   static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
>>   				       const char *source_name,
>>   				       size_t *values_cnt)
>> @@ -861,6 +900,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 = {
>>   	.enable_vblank = rcar_du_crtc_enable_vblank,
>>   	.disable_vblank = rcar_du_crtc_disable_vblank,
>>   	.set_crc_source = rcar_du_crtc_set_crc_source,
>> +	.verify_crc_source = rcar_du_crtc_verify_crc_source,
>>   };
>>
>>   /* ------------------------------------------------------------------------

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/10] drm/rcar-du/crc: Implement verify_crc_source callback

[Laurent Pinchart]

Hi Mahesh,

On Tuesday, 10 July 2018 15:59:11 EEST Kumar, Mahesh wrote:
> On 7/10/2018 5:07 PM, Laurent Pinchart wrote:
> > On Monday, 2 July 2018 14:07:24 EEST Mahesh Kumar wrote:
> >> This patch implements "verify_crc_source" callback function for
> >> rcar drm driver.
> >> 
> >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >> 
> >>   drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 40 +++++++++++++++++++++++++++
> >>   1 file changed, 40 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 15dc9caa128b..24eeaa7e14d7
> >> 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> >> @@ -756,6 +756,45 @@ static void rcar_du_crtc_disable_vblank(struct
> >> drm_crtc *crtc)
> >>  		rcrtc->vblank_enable = false;
> >>   }
> >> 
> >> +static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
> >> +					  const char *source_name,
> >> +					  size_t *values_cnt)
> >> +{
> >> +	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> >> +	unsigned int index = 0;
> >> +	unsigned int i;
> >> +	int ret;
> >> +
> >> +	/*
> >> +	 * Parse the source name. Supported values are "plane%u" to compute the
> >> +	 * CRC on an input plane (%u is the plane ID), and "auto" to compute
> >> the
> >> +	 * CRC on the composer (VSP) output.
> >> +	 */
> >> +	if (!source_name || !strcmp(source_name, "auto")) {
> >> +		goto out;
> >> +	} else if (strstarts(source_name, "plane")) {
> >> +		ret = kstrtouint(source_name + strlen("plane"), 10, &index);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +
> >> +		for (i = 0; i < rcrtc->vsp->num_planes; ++i) {
> >> +			if (index == rcrtc->vsp->planes[i].plane.base.id) {
> >> +				index = i;
> >> +				break;
> >> +			}
> >> +		}
> >> +
> >> +		if (i >= rcrtc->vsp->num_planes)
> >> +			return -EINVAL;
> >> +	} else {
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +out:
> >> +	*values_cnt = 1;
> >> +	return 0;
> > 
> > This duplicates lots of code from the rcar_du_crtc_set_crc_source()
> > function. Could you please extract it to a shared function ?
> 
> Agree, it duplicates the code but "index" is needed by set_crc_source call
> anyway will create a wrapper to avoid duplication of code.

Thank you.

> > Could you please also implement support for the .get_crc_sources()
> > operation ? I think doing so might show limitations in the current API,
> > namely the fact that the list will need to be dynamically created for
> > this driver.
> 
> for that I think rcar_du_crtc_create function can build a dynamic list
> during initializing crtc functions, unless plane can be dynamically
> allocated to any CRTC. this is the place where I need input from maintainer.

That should indeed work. The DU hardware can, in some SoC models, share planes 
between CRTCs. In that case we'll need to ensure that the source corresponds 
to a plane currently associated with the CRTC. That check is currently missing 
and likely out of scope for this patch series, so don't worry about it.

> >> +}
> >> +
> >>   static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
> >>   				       const char *source_name,
> >>   				       size_t *values_cnt)
> >> @@ -861,6 +900,7 @@ static const struct drm_crtc_funcs crtc_funcs_gen3 =
> >> {
> >> 
> >>   	.enable_vblank = rcar_du_crtc_enable_vblank,
> >>   	.disable_vblank = rcar_du_crtc_disable_vblank,
> >>   	.set_crc_source = rcar_du_crtc_set_crc_source,
> >> 
> >> +	.verify_crc_source = rcar_du_crtc_verify_crc_source,
> >> 
> >>   };
> >>   
> >>   /*
> >>   ----------------------------------------------------------------------
> >>   --


-- 
Regards,

Laurent Pinchart



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/10] drm/i915/crc: implement verify_crc_source callback

[Mahesh Kumar]

This patch implements verify_crc_source callback function introduced
earlier in this series.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  |   1 +
 drivers/gpu/drm/i915/intel_drv.h      |   3 +
 drivers/gpu/drm/i915/intel_pipe_crc.c | 108 ++++++++++++++++++++++++++++++++++
 3 files changed, 112 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 56818a45181c..977ee4c7ef7b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12893,6 +12893,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.atomic_duplicate_state = intel_crtc_duplicate_state,
 	.atomic_destroy_state = intel_crtc_destroy_state,
 	.set_crc_source = intel_crtc_set_crc_source,
+	.verify_crc_source = intel_crtc_verify_crc_source,
 };
 
 struct wait_rps_boost {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a6ff2600a3a0..764b53dfe7de 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2156,10 +2156,13 @@ int intel_pipe_crc_create(struct drm_minor *minor);
 #ifdef CONFIG_DEBUG_FS
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt);
+int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
+				 const char *source_name, size_t *values_cnt);
 void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
 void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
 #else
 #define intel_crtc_set_crc_source NULL
+#define intel_crtc_verify_crc_source NULL
 static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
 {
 }
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 39a4e4edda07..a37521380f94 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -913,6 +913,114 @@ int intel_pipe_crc_create(struct drm_minor *minor)
 	return 0;
 }
 
+static int i8xx_crc_source_valid(struct drm_i915_private *dev_priv,
+				 const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int i9xx_crc_source_valid(struct drm_i915_private *dev_priv,
+				 const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_TV:
+	case INTEL_PIPE_CRC_SOURCE_DP_B:
+	case INTEL_PIPE_CRC_SOURCE_DP_C:
+	case INTEL_PIPE_CRC_SOURCE_DP_D:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int vlv_crc_source_valid(struct drm_i915_private *dev_priv,
+				const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_DP_B:
+	case INTEL_PIPE_CRC_SOURCE_DP_C:
+	case INTEL_PIPE_CRC_SOURCE_DP_D:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ilk_crc_source_valid(struct drm_i915_private *dev_priv,
+				const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_PLANE1:
+	case INTEL_PIPE_CRC_SOURCE_PLANE2:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ivb_crc_source_valid(struct drm_i915_private *dev_priv,
+				const enum intel_pipe_crc_source source)
+{
+	switch (source) {
+	case INTEL_PIPE_CRC_SOURCE_PIPE:
+	case INTEL_PIPE_CRC_SOURCE_PLANE1:
+	case INTEL_PIPE_CRC_SOURCE_PLANE2:
+	case INTEL_PIPE_CRC_SOURCE_PF:
+	case INTEL_PIPE_CRC_SOURCE_NONE:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int
+intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
+			  const enum intel_pipe_crc_source source)
+{
+	if (IS_GEN2(dev_priv))
+		return i8xx_crc_source_valid(dev_priv, source);
+	else if (INTEL_GEN(dev_priv) < 5)
+		return i9xx_crc_source_valid(dev_priv, source);
+	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		return vlv_crc_source_valid(dev_priv, source);
+	else if (IS_GEN5(dev_priv) || IS_GEN6(dev_priv))
+		return ilk_crc_source_valid(dev_priv, source);
+	else
+		return ivb_crc_source_valid(dev_priv, source);
+}
+
+int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
+				 size_t *values_cnt)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	enum intel_pipe_crc_source source;
+
+	if (display_crc_ctl_parse_source(source_name, &source) < 0) {
+		DRM_DEBUG_DRIVER("unknown source %s\n", source_name);
+		return -EINVAL;
+	}
+
+	if (source == INTEL_PIPE_CRC_SOURCE_AUTO ||
+	    intel_is_valid_crc_source(dev_priv, source) == 0) {
+		*values_cnt = 5;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt)
 {
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/10] drm/i915/crc: implement verify_crc_source callback

[Maarten Lankhorst]

Op 02-07-18 om 13:07 schreef Mahesh Kumar:
> This patch implements verify_crc_source callback function introduced
> earlier in this series.
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
Patch 6 and 7 were acked for inclusion through drm-misc by Jani:
https://people.freedesktop.org/~cbrill/dri-log/?channel=intel-gfx&highlight_names=&date=2018-07-02

But we still need ack from the other maintainers to get this patch merged. :)
Could you ping a few maintainers?

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/10] drm/i915/crc: implement get_crc_sources callback

[Mahesh Kumar]

This patch implements get_crc_sources callback, which returns list of
all the valid crc sources supported by driver in current platform.

Changes since V1:
 - Return array of crc sources

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  | 1 +
 drivers/gpu/drm/i915/intel_drv.h      | 3 +++
 drivers/gpu/drm/i915/intel_pipe_crc.c | 7 +++++++
 3 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 977ee4c7ef7b..6918c5127473 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12894,6 +12894,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.atomic_destroy_state = intel_crtc_destroy_state,
 	.set_crc_source = intel_crtc_set_crc_source,
 	.verify_crc_source = intel_crtc_verify_crc_source,
+	.get_crc_sources = intel_crtc_get_crc_sources,
 };
 
 struct wait_rps_boost {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 764b53dfe7de..99e3b6477d39 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2158,11 +2158,14 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 			      size_t *values_cnt);
 int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
 				 const char *source_name, size_t *values_cnt);
+const char *const *intel_crtc_get_crc_sources(struct drm_crtc *crtc,
+					      size_t *count);
 void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc);
 void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc);
 #else
 #define intel_crtc_set_crc_source NULL
 #define intel_crtc_verify_crc_source NULL
+#define intel_crtc_get_crc_sources NULL
 static inline void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc)
 {
 }
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index a37521380f94..1dffc346f1bc 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -1001,6 +1001,13 @@ intel_is_valid_crc_source(struct drm_i915_private *dev_priv,
 		return ivb_crc_source_valid(dev_priv, source);
 }
 
+const char *const *intel_crtc_get_crc_sources(struct drm_crtc *crtc,
+					      size_t *count)
+{
+	*count = ARRAY_SIZE(pipe_crc_sources);
+	return pipe_crc_sources;
+}
+
 int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 				 size_t *values_cnt)
 {
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/10] drm/crc: Cleanup crtc_crc_open function

[Mahesh Kumar]

This patch make changes to allocate crc-entries buffer before
enabling CRC generation.
It moves all the failure check early in the function before setting
the source or memory allocation.
Now set_crc_source takes only two variable inputs, values_cnt we
already gets as part of verify_crc_source.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |  3 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  4 +-
 drivers/gpu/drm/drm_debugfs_crc.c                  | 52 +++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h                   |  3 +-
 drivers/gpu/drm/i915/intel_pipe_crc.c              |  4 +-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  5 +--
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  6 +--
 include/drm/drm_crtc.h                             |  3 +-
 8 files changed, 30 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index e43ed064dc46..54056d180003 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -258,8 +258,7 @@ amdgpu_dm_remove_sink_from_freesync_module(struct drm_connector *connector);
 
 /* amdgpu_dm_crc.c */
 #ifdef CONFIG_DEBUG_FS
-int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
-				  size_t *values_cnt);
+int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name);
 int amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc,
 				     const char *src_name,
 				     size_t *values_cnt);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
index dfcca594d52a..e7ad528f5853 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
@@ -62,8 +62,7 @@ amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
 	return 0;
 }
 
-int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
-			   size_t *values_cnt)
+int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 {
 	struct dm_crtc_state *crtc_state = to_dm_crtc_state(crtc->state);
 	struct dc_stream_state *stream_state = crtc_state->stream;
@@ -99,7 +98,6 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
 			return -EINVAL;
 	}
 
-	*values_cnt = 3;
 	/* Reset crc_skipped on dm state */
 	crtc_state->crc_skip_count = 0;
 	return 0;
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index f4d76528d24c..739a813b4b09 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -124,11 +124,9 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	if (source[len] == '\n')
 		source[len] = '\0';
 
-	if (crtc->funcs->verify_crc_source) {
-		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
-		if (ret)
-			return ret;
-	}
+	ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
+	if (ret)
+		return ret;
 
 	spin_lock_irq(&crc->lock);
 
@@ -193,12 +191,15 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 			return ret;
 	}
 
-	if (crtc->funcs->verify_crc_source) {
-		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
-						     &values_cnt);
-		if (ret)
-			return ret;
-	}
+	ret = crtc->funcs->verify_crc_source(crtc, crc->source, &values_cnt);
+	if (ret)
+		return ret;
+
+	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR))
+		return -EINVAL;
+
+	if (WARN_ON(values_cnt == 0))
+		return -EINVAL;
 
 	spin_lock_irq(&crc->lock);
 	if (!crc->opened)
@@ -210,30 +211,22 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 	if (ret)
 		return ret;
 
-	ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
-	if (ret)
-		goto err;
-
-	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) {
-		ret = -EINVAL;
-		goto err_disable;
-	}
-
-	if (WARN_ON(values_cnt == 0)) {
-		ret = -EINVAL;
-		goto err_disable;
-	}
-
 	entries = kcalloc(DRM_CRC_ENTRIES_NR, sizeof(*entries), GFP_KERNEL);
 	if (!entries) {
 		ret = -ENOMEM;
-		goto err_disable;
+		goto err;
 	}
 
 	spin_lock_irq(&crc->lock);
 	crc->entries = entries;
 	crc->values_cnt = values_cnt;
+	spin_unlock_irq(&crc->lock);
 
+	ret = crtc->funcs->set_crc_source(crtc, crc->source);
+	if (ret)
+		goto err;
+
+	spin_lock_irq(&crc->lock);
 	/*
 	 * Only return once we got a first frame, so userspace doesn't have to
 	 * guess when this particular piece of HW will be ready to start
@@ -250,7 +243,7 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 	return 0;
 
 err_disable:
-	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
+	crtc->funcs->set_crc_source(crtc, NULL);
 err:
 	spin_lock_irq(&crc->lock);
 	crtc_crc_cleanup(crc);
@@ -262,9 +255,8 @@ static int crtc_crc_release(struct inode *inode, struct file *filep)
 {
 	struct drm_crtc *crtc = filep->f_inode->i_private;
 	struct drm_crtc_crc *crc = &crtc->crc;
-	size_t values_cnt;
 
-	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
+	crtc->funcs->set_crc_source(crtc, NULL);
 
 	spin_lock_irq(&crc->lock);
 	crtc_crc_cleanup(crc);
@@ -370,7 +362,7 @@ int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
 {
 	struct dentry *crc_ent, *ent;
 
-	if (!crtc->funcs->set_crc_source)
+	if (!crtc->funcs->set_crc_source || !crtc->funcs->verify_crc_source)
 		return 0;
 
 	crc_ent = debugfs_create_dir("crc", crtc->debugfs_entry);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 99e3b6477d39..7fe0341bcc27 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2154,8 +2154,7 @@ void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
 /* intel_pipe_crc.c */
 int intel_pipe_crc_create(struct drm_minor *minor);
 #ifdef CONFIG_DEBUG_FS
-int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
-			      size_t *values_cnt);
+int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name);
 int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
 				 const char *source_name, size_t *values_cnt);
 const char *const *intel_crtc_get_crc_sources(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
index 1dffc346f1bc..27d560f7a817 100644
--- a/drivers/gpu/drm/i915/intel_pipe_crc.c
+++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
@@ -1028,8 +1028,7 @@ int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 	return -EINVAL;
 }
 
-int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
-			      size_t *values_cnt)
+int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
 	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
@@ -1068,7 +1067,6 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
 	}
 
 	pipe_crc->skipped = 0;
-	*values_cnt = 5;
 
 out:
 	intel_display_power_put(dev_priv, power_domain);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 24eeaa7e14d7..829c644addba 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -796,8 +796,7 @@ static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
 }
 
 static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
-				       const char *source_name,
-				       size_t *values_cnt)
+				       const char *source_name)
 {
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 	struct drm_modeset_acquire_ctx ctx;
@@ -837,8 +836,6 @@ static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
-	*values_cnt = 1;
-
 	/* Perform an atomic commit to set the CRC source. */
 	drm_modeset_acquire_init(&ctx, 0);
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 77e91b15ddb4..657372306623 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1117,7 +1117,7 @@ static struct drm_connector *vop_get_edp_connector(struct vop *vop)
 }
 
 static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
-				   const char *source_name, size_t *values_cnt)
+				   const char *source_name)
 {
 	struct vop *vop = to_vop(crtc);
 	struct drm_connector *connector;
@@ -1127,8 +1127,6 @@ static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
 	if (!connector)
 		return -EINVAL;
 
-	*values_cnt = 3;
-
 	if (source_name && strcmp(source_name, "auto") == 0)
 		ret = analogix_dp_start_crc(connector);
 	else if (!source_name)
@@ -1152,7 +1150,7 @@ vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 
 #else
 static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
-				   const char *source_name, size_t *values_cnt)
+				   const char *source_name)
 {
 	return -ENODEV;
 }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index ffaec138aeee..81780325f36f 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -659,8 +659,7 @@ struct drm_crtc_funcs {
 	 *
 	 * 0 on success or a negative error code on failure.
 	 */
-	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
-			      size_t *values_cnt);
+	int (*set_crc_source)(struct drm_crtc *crtc, const char *source);
 	/**
 	 * @verify_crc_source:
 	 *
-- 
2.16.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/10] drm/crc: Cleanup crtc_crc_open function

[Leo Li]

On 2018-07-02 07:07 AM, Mahesh Kumar wrote:
> This patch make changes to allocate crc-entries buffer before
> enabling CRC generation.
> It moves all the failure check early in the function before setting
> the source or memory allocation.
> Now set_crc_source takes only two variable inputs, values_cnt we
> already gets as part of verify_crc_source.
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Acked-by: Leo Li <sunpeng.li@amd.com>

> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |  3 +-
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  4 +-
>   drivers/gpu/drm/drm_debugfs_crc.c                  | 52 +++++++++-------------
>   drivers/gpu/drm/i915/intel_drv.h                   |  3 +-
>   drivers/gpu/drm/i915/intel_pipe_crc.c              |  4 +-
>   drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  5 +--
>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  6 +--
>   include/drm/drm_crtc.h                             |  3 +-
>   8 files changed, 30 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index e43ed064dc46..54056d180003 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -258,8 +258,7 @@ amdgpu_dm_remove_sink_from_freesync_module(struct drm_connector *connector);
>   
>   /* amdgpu_dm_crc.c */
>   #ifdef CONFIG_DEBUG_FS
> -int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> -				  size_t *values_cnt);
> +int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name);
>   int amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc,
>   				     const char *src_name,
>   				     size_t *values_cnt);
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> index dfcca594d52a..e7ad528f5853 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c
> @@ -62,8 +62,7 @@ amdgpu_dm_crtc_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
>   	return 0;
>   }
>   
> -int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
> -			   size_t *values_cnt)
> +int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>   {
>   	struct dm_crtc_state *crtc_state = to_dm_crtc_state(crtc->state);
>   	struct dc_stream_state *stream_state = crtc_state->stream;
> @@ -99,7 +98,6 @@ int amdgpu_dm_crtc_set_crc_source(struct drm_crtc *crtc, const char *src_name,
>   			return -EINVAL;
>   	}
>   
> -	*values_cnt = 3;
>   	/* Reset crc_skipped on dm state */
>   	crtc_state->crc_skip_count = 0;
>   	return 0;
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index f4d76528d24c..739a813b4b09 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -124,11 +124,9 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
>   	if (source[len] == '\n')
>   		source[len] = '\0';
>   
> -	if (crtc->funcs->verify_crc_source) {
> -		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> +	if (ret)
> +		return ret;
>   
>   	spin_lock_irq(&crc->lock);
>   
> @@ -193,12 +191,15 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>   			return ret;
>   	}
>   
> -	if (crtc->funcs->verify_crc_source) {
> -		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
> -						     &values_cnt);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = crtc->funcs->verify_crc_source(crtc, crc->source, &values_cnt);
> +	if (ret)
> +		return ret;
> +
> +	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR))
> +		return -EINVAL;
> +
> +	if (WARN_ON(values_cnt == 0))
> +		return -EINVAL;
>   
>   	spin_lock_irq(&crc->lock);
>   	if (!crc->opened)
> @@ -210,30 +211,22 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>   	if (ret)
>   		return ret;
>   
> -	ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
> -	if (ret)
> -		goto err;
> -
> -	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) {
> -		ret = -EINVAL;
> -		goto err_disable;
> -	}
> -
> -	if (WARN_ON(values_cnt == 0)) {
> -		ret = -EINVAL;
> -		goto err_disable;
> -	}
> -
>   	entries = kcalloc(DRM_CRC_ENTRIES_NR, sizeof(*entries), GFP_KERNEL);
>   	if (!entries) {
>   		ret = -ENOMEM;
> -		goto err_disable;
> +		goto err;
>   	}
>   
>   	spin_lock_irq(&crc->lock);
>   	crc->entries = entries;
>   	crc->values_cnt = values_cnt;
> +	spin_unlock_irq(&crc->lock);
>   
> +	ret = crtc->funcs->set_crc_source(crtc, crc->source);
> +	if (ret)
> +		goto err;
> +
> +	spin_lock_irq(&crc->lock);
>   	/*
>   	 * Only return once we got a first frame, so userspace doesn't have to
>   	 * guess when this particular piece of HW will be ready to start
> @@ -250,7 +243,7 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>   	return 0;
>   
>   err_disable:
> -	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> +	crtc->funcs->set_crc_source(crtc, NULL);
>   err:
>   	spin_lock_irq(&crc->lock);
>   	crtc_crc_cleanup(crc);
> @@ -262,9 +255,8 @@ static int crtc_crc_release(struct inode *inode, struct file *filep)
>   {
>   	struct drm_crtc *crtc = filep->f_inode->i_private;
>   	struct drm_crtc_crc *crc = &crtc->crc;
> -	size_t values_cnt;
>   
> -	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> +	crtc->funcs->set_crc_source(crtc, NULL);
>   
>   	spin_lock_irq(&crc->lock);
>   	crtc_crc_cleanup(crc);
> @@ -370,7 +362,7 @@ int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
>   {
>   	struct dentry *crc_ent, *ent;
>   
> -	if (!crtc->funcs->set_crc_source)
> +	if (!crtc->funcs->set_crc_source || !crtc->funcs->verify_crc_source)
>   		return 0;
>   
>   	crc_ent = debugfs_create_dir("crc", crtc->debugfs_entry);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 99e3b6477d39..7fe0341bcc27 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2154,8 +2154,7 @@ void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
>   /* intel_pipe_crc.c */
>   int intel_pipe_crc_create(struct drm_minor *minor);
>   #ifdef CONFIG_DEBUG_FS
> -int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> -			      size_t *values_cnt);
> +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name);
>   int intel_crtc_verify_crc_source(struct drm_crtc *crtc,
>   				 const char *source_name, size_t *values_cnt);
>   const char *const *intel_crtc_get_crc_sources(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_pipe_crc.c b/drivers/gpu/drm/i915/intel_pipe_crc.c
> index 1dffc346f1bc..27d560f7a817 100644
> --- a/drivers/gpu/drm/i915/intel_pipe_crc.c
> +++ b/drivers/gpu/drm/i915/intel_pipe_crc.c
> @@ -1028,8 +1028,7 @@ int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>   	return -EINVAL;
>   }
>   
> -int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
> -			      size_t *values_cnt)
> +int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>   	struct intel_pipe_crc *pipe_crc = &dev_priv->pipe_crc[crtc->index];
> @@ -1068,7 +1067,6 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name,
>   	}
>   
>   	pipe_crc->skipped = 0;
> -	*values_cnt = 5;
>   
>   out:
>   	intel_display_power_put(dev_priv, power_domain);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 24eeaa7e14d7..829c644addba 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -796,8 +796,7 @@ static int rcar_du_crtc_verify_crc_source(struct drm_crtc *crtc,
>   }
>   
>   static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
> -				       const char *source_name,
> -				       size_t *values_cnt)
> +				       const char *source_name)
>   {
>   	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>   	struct drm_modeset_acquire_ctx ctx;
> @@ -837,8 +836,6 @@ static int rcar_du_crtc_set_crc_source(struct drm_crtc *crtc,
>   		return -EINVAL;
>   	}
>   
> -	*values_cnt = 1;
> -
>   	/* Perform an atomic commit to set the CRC source. */
>   	drm_modeset_acquire_init(&ctx, 0);
>   
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 77e91b15ddb4..657372306623 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1117,7 +1117,7 @@ static struct drm_connector *vop_get_edp_connector(struct vop *vop)
>   }
>   
>   static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
> -				   const char *source_name, size_t *values_cnt)
> +				   const char *source_name)
>   {
>   	struct vop *vop = to_vop(crtc);
>   	struct drm_connector *connector;
> @@ -1127,8 +1127,6 @@ static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
>   	if (!connector)
>   		return -EINVAL;
>   
> -	*values_cnt = 3;
> -
>   	if (source_name && strcmp(source_name, "auto") == 0)
>   		ret = analogix_dp_start_crc(connector);
>   	else if (!source_name)
> @@ -1152,7 +1150,7 @@ vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>   
>   #else
>   static int vop_crtc_set_crc_source(struct drm_crtc *crtc,
> -				   const char *source_name, size_t *values_cnt)
> +				   const char *source_name)
>   {
>   	return -ENODEV;
>   }
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index ffaec138aeee..81780325f36f 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -659,8 +659,7 @@ struct drm_crtc_funcs {
>   	 *
>   	 * 0 on success or a negative error code on failure.
>   	 */
> -	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
> -			      size_t *values_cnt);
> +	int (*set_crc_source)(struct drm_crtc *crtc, const char *source);
>   	/**
>   	 * @verify_crc_source:
>   	 *
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/10] drm/crc: Cleanup crtc_crc_open function

[Laurent Pinchart]

Hi Mahesh,

Thank you for the patch.

On Monday, 2 July 2018 14:07:27 EEST Mahesh Kumar wrote:
> This patch make changes to allocate crc-entries buffer before
> enabling CRC generation.
> It moves all the failure check early in the function before setting
> the source or memory allocation.
> Now set_crc_source takes only two variable inputs, values_cnt we
> already gets as part of verify_crc_source.
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |  3 +-
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  4 +-
>  drivers/gpu/drm/drm_debugfs_crc.c                  | 52 ++++++++-----------
>  drivers/gpu/drm/i915/intel_drv.h                   |  3 +-
>  drivers/gpu/drm/i915/intel_pipe_crc.c              |  4 +-
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  5 +--
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  6 +--
>  include/drm/drm_crtc.h                             |  3 +-
>  8 files changed, 30 insertions(+), 50 deletions(-)

[snip]

> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> b/drivers/gpu/drm/drm_debugfs_crc.c index f4d76528d24c..739a813b4b09 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -124,11 +124,9 @@ static ssize_t crc_control_write(struct file *file,
> const char __user *ubuf, if (source[len] == '\n')
>  		source[len] = '\0';
> 
> -	if (crtc->funcs->verify_crc_source) {
> -		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> +	if (ret)
> +		return ret;
> 
>  	spin_lock_irq(&crc->lock);
> 
> @@ -193,12 +191,15 @@ static int crtc_crc_open(struct inode *inode, struct
> file *filep) return ret;
>  	}
> 
> -	if (crtc->funcs->verify_crc_source) {
> -		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
> -						     &values_cnt);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = crtc->funcs->verify_crc_source(crtc, crc->source, &values_cnt);
> +	if (ret)
> +		return ret;
> +
> +	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR))
> +		return -EINVAL;
> +
> +	if (WARN_ON(values_cnt == 0))
> +		return -EINVAL;
> 
>  	spin_lock_irq(&crc->lock);
>  	if (!crc->opened)
> @@ -210,30 +211,22 @@ static int crtc_crc_open(struct inode *inode, struct
> file *filep) if (ret)
>  		return ret;
> 
> -	ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
> -	if (ret)
> -		goto err;
> -
> -	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) {
> -		ret = -EINVAL;
> -		goto err_disable;
> -	}
> -
> -	if (WARN_ON(values_cnt == 0)) {
> -		ret = -EINVAL;
> -		goto err_disable;
> -	}
> -
>  	entries = kcalloc(DRM_CRC_ENTRIES_NR, sizeof(*entries), GFP_KERNEL);
>  	if (!entries) {
>  		ret = -ENOMEM;
> -		goto err_disable;
> +		goto err;
>  	}

If you moved allocation before the !crc->opened check, you could group the two 
code blocks protected by the crc->lock.

>  	spin_lock_irq(&crc->lock);
>  	crc->entries = entries;
>  	crc->values_cnt = values_cnt;
> +	spin_unlock_irq(&crc->lock);
> 
> +	ret = crtc->funcs->set_crc_source(crtc, crc->source);
> +	if (ret)
> +		goto err;
> +
> +	spin_lock_irq(&crc->lock);
>  	/*
>  	 * Only return once we got a first frame, so userspace doesn't have to
>  	 * guess when this particular piece of HW will be ready to start
> @@ -250,7 +243,7 @@ static int crtc_crc_open(struct inode *inode, struct
> file *filep) return 0;
> 
>  err_disable:
> -	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> +	crtc->funcs->set_crc_source(crtc, NULL);
>  err:
>  	spin_lock_irq(&crc->lock);
>  	crtc_crc_cleanup(crc);
> @@ -262,9 +255,8 @@ static int crtc_crc_release(struct inode *inode, struct
> file *filep) {
>  	struct drm_crtc *crtc = filep->f_inode->i_private;
>  	struct drm_crtc_crc *crc = &crtc->crc;
> -	size_t values_cnt;
> 
> -	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
> +	crtc->funcs->set_crc_source(crtc, NULL);
> 
>  	spin_lock_irq(&crc->lock);
>  	crtc_crc_cleanup(crc);
> @@ -370,7 +362,7 @@ int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
>  {
>  	struct dentry *crc_ent, *ent;
> 
> -	if (!crtc->funcs->set_crc_source)
> +	if (!crtc->funcs->set_crc_source || !crtc->funcs->verify_crc_source)
>  		return 0;
> 
>  	crc_ent = debugfs_create_dir("crc", crtc->debugfs_entry);

[snip]

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/10] drm/crc: Cleanup crtc_crc_open function

[Kumar, Mahesh]

Hi,

Thanks for the review.

On 7/10/2018 5:19 PM, Laurent Pinchart wrote:
> Hi Mahesh,
>
> Thank you for the patch.
>
> On Monday, 2 July 2018 14:07:27 EEST Mahesh Kumar wrote:
>> This patch make changes to allocate crc-entries buffer before
>> enabling CRC generation.
>> It moves all the failure check early in the function before setting
>> the source or memory allocation.
>> Now set_crc_source takes only two variable inputs, values_cnt we
>> already gets as part of verify_crc_source.
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |  3 +-
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  4 +-
>>   drivers/gpu/drm/drm_debugfs_crc.c                  | 52 ++++++++-----------
>>   drivers/gpu/drm/i915/intel_drv.h                   |  3 +-
>>   drivers/gpu/drm/i915/intel_pipe_crc.c              |  4 +-
>>   drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  5 +--
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  6 +--
>>   include/drm/drm_crtc.h                             |  3 +-
>>   8 files changed, 30 insertions(+), 50 deletions(-)
> [snip]
>
>> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
>> b/drivers/gpu/drm/drm_debugfs_crc.c index f4d76528d24c..739a813b4b09 100644
>> --- a/drivers/gpu/drm/drm_debugfs_crc.c
>> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
>> @@ -124,11 +124,9 @@ static ssize_t crc_control_write(struct file *file,
>> const char __user *ubuf, if (source[len] == '\n')
>>   		source[len] = '\0';
>>
>> -	if (crtc->funcs->verify_crc_source) {
>> -		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
>> -		if (ret)
>> -			return ret;
>> -	}
>> +	ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
>> +	if (ret)
>> +		return ret;
>>
>>   	spin_lock_irq(&crc->lock);
>>
>> @@ -193,12 +191,15 @@ static int crtc_crc_open(struct inode *inode, struct
>> file *filep) return ret;
>>   	}
>>
>> -	if (crtc->funcs->verify_crc_source) {
>> -		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
>> -						     &values_cnt);
>> -		if (ret)
>> -			return ret;
>> -	}
>> +	ret = crtc->funcs->verify_crc_source(crtc, crc->source, &values_cnt);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR))
>> +		return -EINVAL;
>> +
>> +	if (WARN_ON(values_cnt == 0))
>> +		return -EINVAL;
>>
>>   	spin_lock_irq(&crc->lock);
>>   	if (!crc->opened)
>> @@ -210,30 +211,22 @@ static int crtc_crc_open(struct inode *inode, struct
>> file *filep) if (ret)
>>   		return ret;
>>
>> -	ret = crtc->funcs->set_crc_source(crtc, crc->source, &values_cnt);
>> -	if (ret)
>> -		goto err;
>> -
>> -	if (WARN_ON(values_cnt > DRM_MAX_CRC_NR)) {
>> -		ret = -EINVAL;
>> -		goto err_disable;
>> -	}
>> -
>> -	if (WARN_ON(values_cnt == 0)) {
>> -		ret = -EINVAL;
>> -		goto err_disable;
>> -	}
>> -
>>   	entries = kcalloc(DRM_CRC_ENTRIES_NR, sizeof(*entries), GFP_KERNEL);
>>   	if (!entries) {
>>   		ret = -ENOMEM;
>> -		goto err_disable;
>> +		goto err;
>>   	}
> If you moved allocation before the !crc->opened check, you could group the two
> code blocks protected by the crc->lock.
agree, will update in next version.
-Mahesh
>>   	spin_lock_irq(&crc->lock);
>>   	crc->entries = entries;
>>   	crc->values_cnt = values_cnt;
>> +	spin_unlock_irq(&crc->lock);
>>
>> +	ret = crtc->funcs->set_crc_source(crtc, crc->source);
>> +	if (ret)
>> +		goto err;
>> +
>> +	spin_lock_irq(&crc->lock);
>>   	/*
>>   	 * Only return once we got a first frame, so userspace doesn't have to
>>   	 * guess when this particular piece of HW will be ready to start
>> @@ -250,7 +243,7 @@ static int crtc_crc_open(struct inode *inode, struct
>> file *filep) return 0;
>>
>>   err_disable:
>> -	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
>> +	crtc->funcs->set_crc_source(crtc, NULL);
>>   err:
>>   	spin_lock_irq(&crc->lock);
>>   	crtc_crc_cleanup(crc);
>> @@ -262,9 +255,8 @@ static int crtc_crc_release(struct inode *inode, struct
>> file *filep) {
>>   	struct drm_crtc *crtc = filep->f_inode->i_private;
>>   	struct drm_crtc_crc *crc = &crtc->crc;
>> -	size_t values_cnt;
>>
>> -	crtc->funcs->set_crc_source(crtc, NULL, &values_cnt);
>> +	crtc->funcs->set_crc_source(crtc, NULL);
>>
>>   	spin_lock_irq(&crc->lock);
>>   	crtc_crc_cleanup(crc);
>> @@ -370,7 +362,7 @@ int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
>>   {
>>   	struct dentry *crc_ent, *ent;
>>
>> -	if (!crtc->funcs->set_crc_source)
>> +	if (!crtc->funcs->set_crc_source || !crtc->funcs->verify_crc_source)
>>   		return 0;
>>
>>   	crc_ent = debugfs_create_dir("crc", crtc->debugfs_entry);
> [snip]
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/10] Revert "drm: crc: Wait for a frame before returning from open()"

[Mahesh Kumar]

This reverts commit e8fa5671183c80342d520ad81d14fa79a9d4a680.

Don't wait for first CRC during crtc_crc_open. It avoids one frame wait
during open. If application want to wait after read call, it can use
poll/read blocking read() call.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 739a813b4b09..add35b77165b 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -226,24 +226,8 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 	if (ret)
 		goto err;
 
-	spin_lock_irq(&crc->lock);
-	/*
-	 * Only return once we got a first frame, so userspace doesn't have to
-	 * guess when this particular piece of HW will be ready to start
-	 * generating CRCs.
-	 */
-	ret = wait_event_interruptible_lock_irq(crc->wq,
-						crtc_crc_data_count(crc),
-						crc->lock);
-	spin_unlock_irq(&crc->lock);
-
-	if (ret)
-		goto err_disable;
-
 	return 0;
 
-err_disable:
-	crtc->funcs->set_crc_source(crtc, NULL);
 err:
 	spin_lock_irq(&crc->lock);
 	crtc_crc_cleanup(crc);
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/10] Revert "drm: crc: Wait for a frame before returning from open()"

[Laurent Pinchart]

Hi Mahesh,

Thank you for the patch.

On Monday, 2 July 2018 14:07:28 EEST Mahesh Kumar wrote:
> This reverts commit e8fa5671183c80342d520ad81d14fa79a9d4a680.
> 
> Don't wait for first CRC during crtc_crc_open. It avoids one frame wait
> during open. If application want to wait after read call, it can use
> poll/read blocking read() call.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> b/drivers/gpu/drm/drm_debugfs_crc.c index 739a813b4b09..add35b77165b 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -226,24 +226,8 @@ static int crtc_crc_open(struct inode *inode, struct
> file *filep) if (ret)
>  		goto err;
> 
> -	spin_lock_irq(&crc->lock);
> -	/*
> -	 * Only return once we got a first frame, so userspace doesn't have to
> -	 * guess when this particular piece of HW will be ready to start
> -	 * generating CRCs.
> -	 */
> -	ret = wait_event_interruptible_lock_irq(crc->wq,
> -						crtc_crc_data_count(crc),
> -						crc->lock);
> -	spin_unlock_irq(&crc->lock);
> -
> -	if (ret)
> -		goto err_disable;
> -
>  	return 0;
> 
> -err_disable:
> -	crtc->funcs->set_crc_source(crtc, NULL);
>  err:
>  	spin_lock_irq(&crc->lock);
>  	crtc_crc_cleanup(crc);


-- 
Regards,

Laurent Pinchart



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/10] drm: crc: Introduce pre_crc_read function

[Mahesh Kumar]

This patch implements a callback function "pre_crc_read" which will
be called before crc read. In this function driver can implement and
preparation work required for successfully reading CRC data.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_debugfs_crc.c |  8 ++++++++
 include/drm/drm_crtc.h            | 14 ++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index add35b77165b..7aeed89f934a 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -272,6 +272,14 @@ static ssize_t crtc_crc_read(struct file *filep, char __user *user_buf,
 		return 0;
 	}
 
+	if (crtc->funcs->pre_crc_read) {
+		ret = crtc->funcs->pre_crc_read(crtc);
+		if (ret) {
+			spin_unlock_irq(&crc->lock);
+			return ret;
+		}
+	}
+
 	/* Nothing to read? */
 	while (crtc_crc_data_count(crc) == 0) {
 		if (filep->f_flags & O_NONBLOCK) {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 81780325f36f..7e2eab9c2f52 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -691,6 +691,20 @@ struct drm_crtc_funcs {
 	 */
 	const char *const *(*get_crc_sources)(struct drm_crtc *crtc,
 					      size_t *count);
+	/**
+	 * @pre_crc_read:
+	 *
+	 * Driver callback for performing any preparation work required by
+	 * driver before reading CRC
+	 *
+	 * This callback is optional if the driver does not support CRC
+	 * generation or no prework is required before reading the crc
+	 *
+	 * RETURNS:
+	 *
+	 * 0 on success or a negative error code on failure.
+	 */
+	int (*pre_crc_read)(struct drm_crtc *crtc);
 
 	/**
 	 * @atomic_print_state:
-- 
2.16.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] drm: crc: Introduce pre_crc_read function

[Laurent Pinchart]

Hi Mahesh,

Thank you for the patch.

On Monday, 2 July 2018 14:07:29 EEST Mahesh Kumar wrote:
> This patch implements a callback function "pre_crc_read" which will
> be called before crc read. In this function driver can implement and
> preparation work required for successfully reading CRC data.

Reviewing this is difficult with a user. Could you submit a patch that makes 
use of this callback in a driver ?

> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_debugfs_crc.c |  8 ++++++++
>  include/drm/drm_crtc.h            | 14 ++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> b/drivers/gpu/drm/drm_debugfs_crc.c index add35b77165b..7aeed89f934a 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -272,6 +272,14 @@ static ssize_t crtc_crc_read(struct file *filep, char
> __user *user_buf, return 0;
>  	}
> 
> +	if (crtc->funcs->pre_crc_read) {
> +		ret = crtc->funcs->pre_crc_read(crtc);
> +		if (ret) {
> +			spin_unlock_irq(&crc->lock);
> +			return ret;
> +		}
> +	}
> +
>  	/* Nothing to read? */
>  	while (crtc_crc_data_count(crc) == 0) {
>  		if (filep->f_flags & O_NONBLOCK) {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 81780325f36f..7e2eab9c2f52 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -691,6 +691,20 @@ struct drm_crtc_funcs {
>  	 */
>  	const char *const *(*get_crc_sources)(struct drm_crtc *crtc,
>  					      size_t *count);
> +	/**
> +	 * @pre_crc_read:
> +	 *
> +	 * Driver callback for performing any preparation work required by
> +	 * driver before reading CRC
> +	 *
> +	 * This callback is optional if the driver does not support CRC
> +	 * generation or no prework is required before reading the crc
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success or a negative error code on failure.
> +	 */
> +	int (*pre_crc_read)(struct drm_crtc *crtc);
> 
>  	/**
>  	 * @atomic_print_state:

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Fi.CI.BAT: failure for Improve crc-core driver interface (rev4)

[Patchwork]

== Series Details ==

Series: Improve crc-core driver interface (rev4)
URL   : https://patchwork.freedesktop.org/series/45246/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4411 -> Patchwork_9494 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_9494 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9494, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/45246/revisions/4/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9494:

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_exec_suspend@basic-s3:
      {fi-cfl-8109u}:     PASS -> DMESG-WARN

    igt@kms_pipe_crc_basic@bad-source:
      {fi-cfl-8109u}:     PASS -> FAIL +1
      fi-skl-6770hq:      PASS -> FAIL +6
      fi-bwr-2160:        PASS -> FAIL +4
      fi-hsw-4770r:       PASS -> FAIL +6
      fi-glk-j4005:       PASS -> FAIL
      fi-cfl-8700k:       PASS -> FAIL +6
      {fi-kbl-x1275}:     PASS -> FAIL
      fi-kbl-guc:         PASS -> FAIL
      fi-glk-dsi:         PASS -> FAIL
      fi-bsw-n3050:       PASS -> FAIL

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
      fi-ivb-3520m:       PASS -> FAIL +6
      fi-cfl-s3:          PASS -> FAIL +6
      fi-skl-6700hq:      PASS -> FAIL +6
      fi-skl-guc:         PASS -> FAIL +6
      fi-blb-e6850:       PASS -> FAIL +4

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-ilk-650:         PASS -> FAIL +4
      fi-elk-e7500:       PASS -> FAIL +4
      fi-byt-n2820:       PASS -> FAIL +4
      fi-snb-2520m:       PASS -> FAIL +4

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
      fi-bdw-5557u:       PASS -> FAIL +6
      fi-pnv-d510:        PASS -> FAIL +4
      fi-skl-6600u:       PASS -> FAIL +6
      fi-bxt-dsi:         PASS -> FAIL +6
      fi-hsw-4770:        PASS -> FAIL +6
      fi-cfl-guc:         PASS -> FAIL +6
      fi-ivb-3770:        PASS -> FAIL +6

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b-frame-sequence:
      fi-hsw-peppy:       PASS -> FAIL +6
      fi-bdw-gvtdvm:      PASS -> FAIL +6
      fi-gdg-551:         PASS -> FAIL +4
      fi-kbl-7500u:       PASS -> FAIL +6
      fi-snb-2600:        PASS -> FAIL +4

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c:
      fi-kbl-7567u:       PASS -> FAIL +6
      fi-skl-6260u:       PASS -> FAIL +6
      fi-skl-6700k2:      PASS -> FAIL +6
      fi-skl-gvtdvm:      PASS -> FAIL +6
      fi-bxt-j4205:       PASS -> FAIL +6
      fi-kbl-7560u:       PASS -> FAIL +6
      fi-whl-u:           PASS -> FAIL +6
      fi-kbl-r:           PASS -> FAIL +6

    
    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_9494 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c:
      fi-glk-j4005:       PASS -> FAIL (fdo#106211) +5
      fi-bsw-n3050:       PASS -> FAIL (fdo#106211) +1

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-c-frame-sequence:
      fi-glk-dsi:         PASS -> FAIL (fdo#106211) +5

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#106211 https://bugs.freedesktop.org/show_bug.cgi?id=106211


== Participating hosts (44 -> 39) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4411 -> Patchwork_9494

  CI_DRM_4411: 3fd8031a8f5d378ebda63131f6b504c3c4a5eb93 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4532: 840d12e2f050b784552197403d6575a57b6e896d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9494: 43533286914804b8dd5bc3d683945a5bce9be6e6 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

435332869148 drm: crc: Introduce pre_crc_read function
39240b6ce9c5 Revert "drm: crc: Wait for a frame before returning from open()"
92d712d5c8bf drm/crc: Cleanup crtc_crc_open function
f1a6ac764e69 drm/i915/crc: implement get_crc_sources callback
29e0bbaabf60 drm/i915/crc: implement verify_crc_source callback
c75c0d05c696 drm/rcar-du/crc: Implement verify_crc_source callback
649d027b2b34 drm/amdgpu_dm/crc: Implement verify_crc_source callback
8993123ebcb1 drm/rockchip/crc: Implement verify_crc_source callback
d5dac505c466 drm: crc: Introduce get_crc_sources callback
2fe8786784e0 drm: crc: Introduce verify_crc_source callback

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9494/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/10] Improve crc-core driver interface

[Alex Deucher]

+ Harry and Leo


On Mon, Jul 2, 2018 at 7:07 AM, Mahesh Kumar <mahesh1.kumar@intel.com> wrote:
> This series improves crc-core <-> driver interface.
> This series adds following functionality in the crc-core
>  - Now control node will print all the available sources if
>    implemented by driver along with current source.
>  - Setting of sorce will fail if provided source is not supported
>  - cleanup of crtc_crc_open function first allocate memory before
>    enabling CRC generation
>  - Don't block open() call instead wait in crc read call.
>
> Following IGT will fail due to crc-core <-> driver interface change
> igt@kms_pipe_crc_basic@bad-source <now setting bad-source itself will fail>
> ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X
> ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X-frame-sequence
> In nonblocking crc tests we'll get lesser crc's due to skipping crc
>
> AMD/Rockchip/rcar code path is not validated and need inputs
>
> Changes:
>  - Add dri-devel in Cc
> Changes rev2:
>  - now get_crc_sources returns a constant pointer to an array of
>    source list and crc-core does the verification
> Changes rev3:
>  - reorg patches to push non r-b patches to the last
>  - add r-b tag
>
> Cc: dri-devel@lists.freedesktop.org
>
> Mahesh Kumar (10):
>   drm: crc: Introduce verify_crc_source callback
>   drm: crc: Introduce get_crc_sources callback
>   drm/rockchip/crc: Implement verify_crc_source callback
>   drm/amdgpu_dm/crc: Implement verify_crc_source callback
>   drm/rcar-du/crc: Implement verify_crc_source callback
>   drm/i915/crc: implement verify_crc_source callback
>   drm/i915/crc: implement get_crc_sources callback
>   drm/crc: Cleanup crtc_crc_open function
>   Revert "drm: crc: Wait for a frame before returning from open()"
>   drm: crc: Introduce pre_crc_read function
>
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |   1 +
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   7 +-
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  20 +++-
>  drivers/gpu/drm/drm_debugfs_crc.c                  |  79 ++++++++------
>  drivers/gpu/drm/i915/intel_display.c               |   2 +
>  drivers/gpu/drm/i915/intel_drv.h                   |   9 +-
>  drivers/gpu/drm/i915/intel_pipe_crc.c              | 119 ++++++++++++++++++++-
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  45 +++++++-
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  26 ++++-
>  include/drm/drm_crtc.h                             |  48 ++++++++-
>  10 files changed, 305 insertions(+), 51 deletions(-)
>
> --
> 2.16.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 00/10] Improve crc-core driver interface

[Mahesh Kumar]

This series improves crc-core <-> driver interface.
This series adds following functionality in the crc-core
 - Now control node will print all the available sources if
   implemented by driver along with current source.
 - Setting of sorce will fail if provided source is not supported
 - cleanup of crtc_crc_open function first allocate memory before
   enabling CRC generation
 - Don't block open() call instead wait in crc read call.

Following IGT will fail due to crc-core <-> driver interface change
igt@kms_pipe_crc_basic@bad-source <now setting bad-source itself will fail>
ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X 
ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X-frame-sequence
In nonblocking crc tests we'll get lesser crc's due to skipping crc

AMD/Rockchip/rcar code path is not validated and need inputs

Cc: dri-devel@lists.freedesktop.org

Mahesh Kumar (10):
  drm: crc: Introduce verify_crc_source callback
  drm: crc: Introduce get_crc_sources callback
  drm/rockchip/crc: Implement verify_crc_source callback
  drm/amdgpu_dm/crc: Implement verify_crc_source callback
  drm/rcar-du/crc: Implement verify_crc_source callback
  drm/i915/crc: implement verify_crc_source callback
  drm/i915/crc: implement get_crc_sources callback
  drm/crc: Cleanup crtc_crc_open function
  Revert "drm: crc: Wait for a frame before returning from open()"
  drm/rcar-du/crc: Implement get_crc_sources callback

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |   1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   7 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  20 +++-
 drivers/gpu/drm/drm_debugfs_crc.c                  |  92 +++++++++-------
 drivers/gpu/drm/i915/intel_display.c               |   2 +
 drivers/gpu/drm/i915/intel_drv.h                   |   9 +-
 drivers/gpu/drm/i915/intel_pipe_crc.c              | 119 ++++++++++++++++++++-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  82 +++++++++++---
 drivers/gpu/drm/rcar-du/rcar_du_drv.c              |   2 +
 drivers/gpu/drm/rcar-du/rcar_du_drv.h              |   2 +
 drivers/gpu/drm/rcar-du/rcar_du_kms.c              |  67 ++++++++++++
 drivers/gpu/drm/rcar-du/rcar_du_kms.h              |   1 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  26 ++++-
 include/drm/drm_crtc.h                             |  40 ++++++-
 14 files changed, 396 insertions(+), 74 deletions(-)

-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/10] drm: crc: Introduce verify_crc_source callback

[Mahesh Kumar]

This patch adds a new callback function "verify_crc_source" which will
be used during setting the crc source in control node. This will help
in avoiding setting of wrong string for source.

Changes since V1:
 - do not yet verify_crc_source during open.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c |  8 ++++++++
 include/drm/drm_crtc.h            | 15 +++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 99961192bf03..1956acb61cc8 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	struct drm_crtc *crtc = m->private;
 	struct drm_crtc_crc *crc = &crtc->crc;
 	char *source;
+	size_t values_cnt;
+	int ret = 0;
 
 	if (len == 0)
 		return 0;
@@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	if (source[len] == '\n')
 		source[len] = '\0';
 
+	if (crtc->funcs->verify_crc_source) {
+		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
+		if (ret)
+			return ret;
+	}
+
 	spin_lock_irq(&crc->lock);
 
 	if (crc->opened) {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 17f4f93340b8..3ce5d2fe133d 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -684,6 +684,21 @@ struct drm_crtc_funcs {
 	 */
 	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
 			      size_t *values_cnt);
+	/**
+	 * @verify_crc_source:
+	 *
+	 * verifies the source of CRC checksums of frames before setting the
+	 * source for CRC and during crc open.
+	 *
+	 * This callback is optional if the driver does not support any CRC
+	 * generation functionality.
+	 *
+	 * RETURNS:
+	 *
+	 * 0 on success or a negative error code on failure.
+	 */
+	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
+				 size_t *values_cnt);
 
 	/**
 	 * @atomic_print_state:
-- 
2.16.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/10] drm: crc: Introduce verify_crc_source callback

[Laurent Pinchart]

Hi Mahesh,

Thank you for the patch.

On Thursday, 12 July 2018 11:36:26 EEST Mahesh Kumar wrote:
> This patch adds a new callback function "verify_crc_source" which will
> be used during setting the crc source in control node. This will help
> in avoiding setting of wrong string for source.
> 
> Changes since V1:
>  - do not yet verify_crc_source during open.
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_debugfs_crc.c |  8 ++++++++
>  include/drm/drm_crtc.h            | 15 +++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
> b/drivers/gpu/drm/drm_debugfs_crc.c index 99961192bf03..1956acb61cc8 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const
> char __user *ubuf, struct drm_crtc *crtc = m->private;
>  	struct drm_crtc_crc *crc = &crtc->crc;
>  	char *source;
> +	size_t values_cnt;
> +	int ret = 0;

No need to initialize ret to 0 here.

> 
>  	if (len == 0)
>  		return 0;
> @@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file,
> const char __user *ubuf, if (source[len] == '\n')
>  		source[len] = '\0';
> 
> +	if (crtc->funcs->verify_crc_source) {
> +		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	spin_lock_irq(&crc->lock);
> 
>  	if (crc->opened) {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 17f4f93340b8..3ce5d2fe133d 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -684,6 +684,21 @@ struct drm_crtc_funcs {
>  	 */
>  	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
>  			      size_t *values_cnt);
> +	/**
> +	 * @verify_crc_source:
> +	 *
> +	 * verifies the source of CRC checksums of frames before setting the
> +	 * source for CRC and during crc open.

Please also document whether the source parameter can be NULL.

It would also be nice to document values_cnt, but as that's not documented for 
the .set_crc_source() operation at the moment, I won't block the patch for 
this.

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	 *
> +	 * This callback is optional if the driver does not support any CRC
> +	 * generation functionality.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success or a negative error code on failure.
> +	 */
> +	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
> +				 size_t *values_cnt);
> 
>  	/**
>  	 * @atomic_print_state:

-- 
Regards,

Laurent Pinchart



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 00/10] Improve crc-core driver interface

[Mahesh Kumar]

This series improves crc-core <-> driver interface.
This series adds following functionality in the crc-core
 - Now control node will print all the available sources if
   implemented by driver along with current source.
 - Setting of sorce will fail if provided source is not supported
 - cleanup of crtc_crc_open function first allocate memory before
   enabling CRC generation
 - Don't block open() call instead wait in crc read call.

Following IGT will fail due to crc-core <-> driver interface change
igt@kms_pipe_crc_basic@bad-source <now setting bad-source itself will fail>
ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X 
ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X-frame-sequence
In nonblocking crc tests we'll get lesser crc's due to skipping crc

AMD/Rockchip/rcar code path is not validated and need inputs

Mahesh Kumar (10):
  drm: crc: Introduce verify_crc_source callback
  drm: crc: Introduce get_crc_sources callback
  drm/rockchip/crc: Implement verify_crc_source callback
  drm/amdgpu_dm/crc: Implement verify_crc_source callback
  drm/rcar-du/crc: Implement verify_crc_source callback
  drm/i915/crc: implement verify_crc_source callback
  drm/i915/crc: implement get_crc_sources callback
  drm/crc: Cleanup crtc_crc_open function
  Revert "drm: crc: Wait for a frame before returning from open()"
  drm/rcar-du/crc: Implement get_crc_sources callback

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |   1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   7 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  20 +++-
 drivers/gpu/drm/drm_debugfs_crc.c                  |  92 +++++++++-------
 drivers/gpu/drm/i915/intel_display.c               |   2 +
 drivers/gpu/drm/i915/intel_drv.h                   |   9 +-
 drivers/gpu/drm/i915/intel_pipe_crc.c              | 119 ++++++++++++++++++++-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  82 +++++++++++---
 drivers/gpu/drm/rcar-du/rcar_du_drv.c              |   2 +
 drivers/gpu/drm/rcar-du/rcar_du_drv.h              |   2 +
 drivers/gpu/drm/rcar-du/rcar_du_kms.c              |  67 ++++++++++++
 drivers/gpu/drm/rcar-du/rcar_du_kms.h              |   1 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  26 ++++-
 include/drm/drm_crtc.h                             |  40 ++++++-
 14 files changed, 396 insertions(+), 74 deletions(-)

-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/10] drm: crc: Introduce verify_crc_source callback

[Mahesh Kumar]

This patch adds a new callback function "verify_crc_source" which will
be used during setting the crc source in control node. This will help
in avoiding setting of wrong string for source.

Changes since V1:
 - do not yet verify_crc_source during open.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c |  8 ++++++++
 include/drm/drm_crtc.h            | 15 +++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 99961192bf03..1956acb61cc8 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	struct drm_crtc *crtc = m->private;
 	struct drm_crtc_crc *crc = &crtc->crc;
 	char *source;
+	size_t values_cnt;
+	int ret = 0;
 
 	if (len == 0)
 		return 0;
@@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	if (source[len] == '\n')
 		source[len] = '\0';
 
+	if (crtc->funcs->verify_crc_source) {
+		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
+		if (ret)
+			return ret;
+	}
+
 	spin_lock_irq(&crc->lock);
 
 	if (crc->opened) {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 17f4f93340b8..3ce5d2fe133d 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -684,6 +684,21 @@ struct drm_crtc_funcs {
 	 */
 	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
 			      size_t *values_cnt);
+	/**
+	 * @verify_crc_source:
+	 *
+	 * verifies the source of CRC checksums of frames before setting the
+	 * source for CRC and during crc open.
+	 *
+	 * This callback is optional if the driver does not support any CRC
+	 * generation functionality.
+	 *
+	 * RETURNS:
+	 *
+	 * 0 on success or a negative error code on failure.
+	 */
+	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
+				 size_t *values_cnt);
 
 	/**
 	 * @atomic_print_state:
-- 
2.16.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 00/10] Improve crc-core driver interface

[Mahesh Kumar]

This series improves crc-core <-> driver interface.
This series adds following functionality in the crc-core
 - Now control node will print all the available sources if
   implemented by driver along with current source.
 - Setting of sorce will fail if provided source is not supported
 - cleanup of crtc_crc_open function first allocate memory before
   enabling CRC generation
 - Don't block open() call instead wait in crc read call.

Following IGT will fail due to crc-core <-> driver interface change
igt@kms_pipe_crc_basic@bad-source <now setting bad-source itself will fail>
ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X 
ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X-frame-sequence
In nonblocking crc tests we'll get lesser crc's due to skipping crc

AMD/Rockchip/rcar code path is not validated and need inputs

Changes:
 - Add dri-devel in Cc
Changes rev2:
 - now get_crc_sources returns a constant pointer to an array of
   source list and crc-core does the verification

Cc: dri-devel@lists.freedesktop.org

Mahesh Kumar (10):
  drm: crc: Introduce verify_crc_source callback
  drm: crc: Introduce pre_crc_read function
  drm: crc: Introduce get_crc_sources callback
  drm/rockchip/crc: Implement verify_crc_source callback
  drm/amdgpu_dm/crc: Implement verify_crc_source callback
  drm/rcar-du/crc: Implement verify_crc_source callback
  drm/i915/crc: implement verify_crc_source callback
  drm/i915/crc: implement get_crc_sources callback
  drm/crc: Cleanup crtc_crc_open function
  Revert "drm: crc: Wait for a frame before returning from open()"

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |   1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   7 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  20 +++-
 drivers/gpu/drm/drm_debugfs_crc.c                  |  78 ++++++++------
 drivers/gpu/drm/i915/intel_display.c               |   2 +
 drivers/gpu/drm/i915/intel_drv.h                   |   8 +-
 drivers/gpu/drm/i915/intel_pipe_crc.c              | 118 ++++++++++++++++++++-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  45 +++++++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  26 ++++-
 include/drm/drm_crtc.h                             |  47 +++++++-
 10 files changed, 301 insertions(+), 51 deletions(-)

-- 
2.16.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/10] drm: crc: Introduce verify_crc_source callback

[Mahesh Kumar]

This patch adds a new callback function "verify_crc_source" which will
be used during setting the crc source in control node and while opening
data node for crc reading. This will help in avoiding setting of wrong
string for source.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++
 include/drm/drm_crtc.h            | 15 +++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 9f8312137cad..c6a725b79ac6 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	struct drm_crtc *crtc = m->private;
 	struct drm_crtc_crc *crc = &crtc->crc;
 	char *source;
+	size_t values_cnt;
+	int ret = 0;
 
 	if (len == 0)
 		return 0;
@@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	if (source[len] == '\n')
 		source[len] = '\0';
 
+	if (crtc->funcs->verify_crc_source) {
+		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
+		if (ret)
+			return ret;
+	}
+
 	spin_lock_irq(&crc->lock);
 
 	if (crc->opened) {
@@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 			return ret;
 	}
 
+	if (crtc->funcs->verify_crc_source) {
+		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
+						     &values_cnt);
+		if (ret)
+			return ret;
+	}
+
 	spin_lock_irq(&crc->lock);
 	if (!crc->opened)
 		crc->opened = true;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 23eddbccab10..1a6dcbf91744 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -661,6 +661,21 @@ struct drm_crtc_funcs {
 	 */
 	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
 			      size_t *values_cnt);
+	/**
+	 * @verify_crc_source:
+	 *
+	 * verifies the source of CRC checksums of frames before setting the
+	 * source for CRC and during crc open.
+	 *
+	 * This callback is optional if the driver does not support any CRC
+	 * generation functionality.
+	 *
+	 * RETURNS:
+	 *
+	 * 0 on success or a negative error code on failure.
+	 */
+	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
+				 size_t *values_cnt);
 
 	/**
 	 * @atomic_print_state:
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 00/10] Improve crc-core driver interface

[Mahesh Kumar]

This series improves crc-core <-> driver interface.
This series adds following functionality in the crc-core
 - Now control node will print all the available sources if
   implemented by driver along with current source.
 - Setting of sorce will fail if provided source is not supported
 - cleanup of crtc_crc_open function first allocate memory before
   enabling CRC generation
 - Don't block open() call instead wait in crc read call.

Following IGT will fail due to crc-core <-> driver interface change
igt@kms_pipe_crc_basic@bad-source <now setting bad-source itself will fail>
ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X 
ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X-frame-sequence
In nonblocking crc tests we'll get lesser crc's due to skipping crc

AMD/Rockchip/rcar code path is not validated and need inputs

Changes:
 - Add dri-devel in Cc

Mahesh Kumar (10):
  drm: crc: Introduce verify_crc_source callback
  drm: crc: Introduce pre_crc_read function
  drm: crc: Introduce get_crc_sources callback
  drm/rockchip/crc: Implement verify_crc_source callback
  drm/amdgpu_dm/crc: Implement verify_crc_source callback
  drm/rcar-du/crc: Implement verify_crc_source callback
  drm/i915/crc: implement verify_crc_source callback
  drm/i915/crc: implement get_crc_sources callback
  drm/crc: Cleanup crtc_crc_open function
  Revert "drm: crc: Wait for a frame before returning from open()"

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |   1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   7 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  20 +++-
 drivers/gpu/drm/drm_debugfs_crc.c                  |  62 ++++++-----
 drivers/gpu/drm/i915/intel_display.c               |   2 +
 drivers/gpu/drm/i915/intel_drv.h                   |   8 +-
 drivers/gpu/drm/i915/intel_pipe_crc.c              | 124 ++++++++++++++++++++-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  45 +++++++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  27 ++++-
 include/drm/drm_crtc.h                             |  42 ++++++-
 10 files changed, 288 insertions(+), 50 deletions(-)

-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/10] drm: crc: Introduce verify_crc_source callback

[Mahesh Kumar]

This patch adds a new callback function "verify_crc_source" which will
be used during setting the crc source in control node and while opening
data node for crc reading. This will help in avoiding setting of wrong
string for source.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++
 include/drm/drm_crtc.h            | 15 +++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 9f8312137cad..c6a725b79ac6 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	struct drm_crtc *crtc = m->private;
 	struct drm_crtc_crc *crc = &crtc->crc;
 	char *source;
+	size_t values_cnt;
+	int ret = 0;
 
 	if (len == 0)
 		return 0;
@@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	if (source[len] == '\n')
 		source[len] = '\0';
 
+	if (crtc->funcs->verify_crc_source) {
+		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
+		if (ret)
+			return ret;
+	}
+
 	spin_lock_irq(&crc->lock);
 
 	if (crc->opened) {
@@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 			return ret;
 	}
 
+	if (crtc->funcs->verify_crc_source) {
+		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
+						     &values_cnt);
+		if (ret)
+			return ret;
+	}
+
 	spin_lock_irq(&crc->lock);
 	if (!crc->opened)
 		crc->opened = true;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 23eddbccab10..1a6dcbf91744 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -661,6 +661,21 @@ struct drm_crtc_funcs {
 	 */
 	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
 			      size_t *values_cnt);
+	/**
+	 * @verify_crc_source:
+	 *
+	 * verifies the source of CRC checksums of frames before setting the
+	 * source for CRC and during crc open.
+	 *
+	 * This callback is optional if the driver does not support any CRC
+	 * generation functionality.
+	 *
+	 * RETURNS:
+	 *
+	 * 0 on success or a negative error code on failure.
+	 */
+	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
+				 size_t *values_cnt);
 
 	/**
 	 * @atomic_print_state:
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 00/10] Improve crc-core driver interface

[Mahesh Kumar]

This series improves crc-core <-> driver interface.
This series adds following functionality in the crc-core
 - Now control node will print all the available sources if
   implemented by driver along with current source.
 - Setting of sorce will fail if provided source is not supported
 - cleanup of crtc_crc_open function first allocate memory before
   enabling CRC generation
 - Don't block open() call instead wait in crc read call.

Following IGT will fail due to crc-core <-> driver interface change
igt@kms_pipe_crc_basic@bad-source <now setting bad-source itself will fail>
ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X 
ig@kms_pipe_crc_basic@nonblocking-crc-pipe-X-frame-sequence
In nonblocking crc tests we'll get lesser crc's due to skipping crc

AMD/Rockchip/rcar code path is not validated and need inputs

Mahesh Kumar (10):
  drm: crc: Introduce verify_crc_source callback
  drm: crc: Introduce pre_crc_read function
  drm: crc: Introduce get_crc_sources callback
  drm/rockchip/crc: Implement verify_crc_source callback
  drm/amdgpu_dm/crc: Implement verify_crc_source callback
  drm/rcar-du/crc: Implement verify_crc_source callback
  drm/i915/crc: implement verify_crc_source callback
  drm/i915/crc: implement get_crc_sources callback
  drm/crc: Cleanup crtc_crc_open function
  Revert "drm: crc: Wait for a frame before returning from open()"

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  |   1 +
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h  |   6 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crc.c  |  20 +++-
 drivers/gpu/drm/drm_debugfs_crc.c                  |  62 ++++++-----
 drivers/gpu/drm/i915/intel_display.c               |   2 +
 drivers/gpu/drm/i915/intel_drv.h                   |   8 +-
 drivers/gpu/drm/i915/intel_pipe_crc.c              | 124 ++++++++++++++++++++-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c             |  47 +++++++-
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |  23 +++-
 include/drm/drm_crtc.h                             |  42 ++++++-
 10 files changed, 286 insertions(+), 49 deletions(-)

-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/10] drm: crc: Introduce verify_crc_source callback

[Mahesh Kumar]

This patch adds a new callback function "verify_crc_source" which will
be used during setting the crc source in control node and while opening
data node for crc reading. This will help in avoiding setting of wrong
string for source.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++
 include/drm/drm_crtc.h            | 15 +++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
index 9f8312137cad..c6a725b79ac6 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	struct drm_crtc *crtc = m->private;
 	struct drm_crtc_crc *crc = &crtc->crc;
 	char *source;
+	size_t values_cnt;
+	int ret = 0;
 
 	if (len == 0)
 		return 0;
@@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
 	if (source[len] == '\n')
 		source[len] = '\0';
 
+	if (crtc->funcs->verify_crc_source) {
+		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
+		if (ret)
+			return ret;
+	}
+
 	spin_lock_irq(&crc->lock);
 
 	if (crc->opened) {
@@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
 			return ret;
 	}
 
+	if (crtc->funcs->verify_crc_source) {
+		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
+						     &values_cnt);
+		if (ret)
+			return ret;
+	}
+
 	spin_lock_irq(&crc->lock);
 	if (!crc->opened)
 		crc->opened = true;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 23eddbccab10..1a6dcbf91744 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -661,6 +661,21 @@ struct drm_crtc_funcs {
 	 */
 	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
 			      size_t *values_cnt);
+	/**
+	 * @verify_crc_source:
+	 *
+	 * verifies the source of CRC checksums of frames before setting the
+	 * source for CRC and during crc open.
+	 *
+	 * This callback is optional if the driver does not support any CRC
+	 * generation functionality.
+	 *
+	 * RETURNS:
+	 *
+	 * 0 on success or a negative error code on failure.
+	 */
+	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
+				 size_t *values_cnt);
 
 	/**
 	 * @atomic_print_state:
-- 
2.16.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/10] drm: crc: Introduce verify_crc_source callback

[Daniel Vetter]

On Fri, Jun 22, 2018 at 04:11:25PM +0530, Mahesh Kumar wrote:
> This patch adds a new callback function "verify_crc_source" which will
> be used during setting the crc source in control node and while opening
> data node for crc reading. This will help in avoiding setting of wrong
> string for source.
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>

drm core patches must be cc'ed to dri-devel. simplest way to do that and
never forget is to add a Cc: line to the commit message.
-Daniel

> ---
>  drivers/gpu/drm/drm_debugfs_crc.c | 15 +++++++++++++++
>  include/drm/drm_crtc.h            | 15 +++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 9f8312137cad..c6a725b79ac6 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -87,6 +87,8 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
>  	struct drm_crtc *crtc = m->private;
>  	struct drm_crtc_crc *crc = &crtc->crc;
>  	char *source;
> +	size_t values_cnt;
> +	int ret = 0;
>  
>  	if (len == 0)
>  		return 0;
> @@ -104,6 +106,12 @@ static ssize_t crc_control_write(struct file *file, const char __user *ubuf,
>  	if (source[len] == '\n')
>  		source[len] = '\0';
>  
> +	if (crtc->funcs->verify_crc_source) {
> +		ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	spin_lock_irq(&crc->lock);
>  
>  	if (crc->opened) {
> @@ -167,6 +175,13 @@ static int crtc_crc_open(struct inode *inode, struct file *filep)
>  			return ret;
>  	}
>  
> +	if (crtc->funcs->verify_crc_source) {
> +		ret = crtc->funcs->verify_crc_source(crtc, crc->source,
> +						     &values_cnt);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	spin_lock_irq(&crc->lock);
>  	if (!crc->opened)
>  		crc->opened = true;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 23eddbccab10..1a6dcbf91744 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -661,6 +661,21 @@ struct drm_crtc_funcs {
>  	 */
>  	int (*set_crc_source)(struct drm_crtc *crtc, const char *source,
>  			      size_t *values_cnt);
> +	/**
> +	 * @verify_crc_source:
> +	 *
> +	 * verifies the source of CRC checksums of frames before setting the
> +	 * source for CRC and during crc open.
> +	 *
> +	 * This callback is optional if the driver does not support any CRC
> +	 * generation functionality.
> +	 *
> +	 * RETURNS:
> +	 *
> +	 * 0 on success or a negative error code on failure.
> +	 */
> +	int (*verify_crc_source)(struct drm_crtc *crtc, const char *source,
> +				 size_t *values_cnt);
>  
>  	/**
>  	 * @atomic_print_state:
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx