diff mbox series

[2/3] atomisp: sh_css_params: write the sp_group config according to the ISP model

Message ID 20230605102903.924283-3-hpa@redhat.com (mailing list archive)
State Superseded
Headers
Series Remove #ifdef ISP2401 and unifying sh_css_sp_group structure |

Commit Message

Kate Hsuan June 5, 2023, 10:29 a.m. UTC 
  Pick up the necessary part of sp_group configuration for both model and
then copy those parts into a tempetory buffer. This buffer is finally
written to the ISP with correct length.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 .../staging/media/atomisp/pci/sh_css_params.c | 37 ++++++++++++++++++-
 1 file changed, 35 insertions(+), 2 deletions(-)

Comments

Dan Carpenter June 5, 2023, 11:10 a.m. UTC | #1 
On Mon, Jun 05, 2023 at 06:29:02PM +0800, Kate Hsuan wrote:
> Pick up the necessary part of sp_group configuration for both model and
> then copy those parts into a tempetory buffer. This buffer is finally
> written to the ISP with correct length.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>  .../staging/media/atomisp/pci/sh_css_params.c | 37 ++++++++++++++++++-
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/sh_css_params.c b/drivers/staging/media/atomisp/pci/sh_css_params.c
> index 588f2adab058..2913d9d6d226 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css_params.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css_params.c
> @@ -3720,10 +3720,43 @@ struct ia_css_shading_table *ia_css_get_shading_table(struct ia_css_stream
>  
>  ia_css_ptr sh_css_store_sp_group_to_ddr(void)
>  {
> +	u8 *write_buf;
> +	u8 *buf_ptr;
> +
>  	IA_CSS_ENTER_LEAVE_PRIVATE("void");
> +
> +	write_buf = kzalloc(sizeof(u8) * 8192, GFP_KERNEL);

Please add a check for allocation failure.

regards,
dan carpenter


> +
> +	buf_ptr = write_buf;
> +	if (IS_ISP2401) {
> +		memcpy(buf_ptr, &sh_css_sp_group.config, sizeof(u8) * 5);
> +		buf_ptr += (sizeof(u8) * 5);
> +		memset(buf_ptr, 0, 3);
> +		buf_ptr += 3; /* Padding 3 bytes for struct sh_css_sp_config*/
> +	} else {
> +		memcpy(buf_ptr, &sh_css_sp_group.config, sizeof(sh_css_sp_group.config));
> +		buf_ptr += sizeof(sh_css_sp_group.config);
> +	}
Kate Hsuan June 6, 2023, 2:34 a.m. UTC | #2 
Hi,

On Mon, Jun 5, 2023 at 7:11 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Mon, Jun 05, 2023 at 06:29:02PM +0800, Kate Hsuan wrote:
> > Pick up the necessary part of sp_group configuration for both model and
> > then copy those parts into a tempetory buffer. This buffer is finally
> > written to the ISP with correct length.
> >
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
> > ---
> >  .../staging/media/atomisp/pci/sh_css_params.c | 37 ++++++++++++++++++-
> >  1 file changed, 35 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/sh_css_params.c b/drivers/staging/media/atomisp/pci/sh_css_params.c
> > index 588f2adab058..2913d9d6d226 100644
> > --- a/drivers/staging/media/atomisp/pci/sh_css_params.c
> > +++ b/drivers/staging/media/atomisp/pci/sh_css_params.c
> > @@ -3720,10 +3720,43 @@ struct ia_css_shading_table *ia_css_get_shading_table(struct ia_css_stream
> >
> >  ia_css_ptr sh_css_store_sp_group_to_ddr(void)
> >  {
> > +     u8 *write_buf;
> > +     u8 *buf_ptr;
> > +
> >       IA_CSS_ENTER_LEAVE_PRIVATE("void");
> > +
> > +     write_buf = kzalloc(sizeof(u8) * 8192, GFP_KERNEL);
>
> Please add a check for allocation failure.
>
> regards,
> dan carpenter

Thank you for reviewing this.
I will fix it in the v2 patch.

>
>
> > +
> > +     buf_ptr = write_buf;
> > +     if (IS_ISP2401) {
> > +             memcpy(buf_ptr, &sh_css_sp_group.config, sizeof(u8) * 5);
> > +             buf_ptr += (sizeof(u8) * 5);
> > +             memset(buf_ptr, 0, 3);
> > +             buf_ptr += 3; /* Padding 3 bytes for struct sh_css_sp_config*/
> > +     } else {
> > +             memcpy(buf_ptr, &sh_css_sp_group.config, sizeof(sh_css_sp_group.config));
> > +             buf_ptr += sizeof(sh_css_sp_group.config);
> > +     }
>
Hans de Goede June 6, 2023, 11:02 a.m. UTC | #3 
Hi Kate,

On 6/5/23 12:29, Kate Hsuan wrote:
> Pick up the necessary part of sp_group configuration for both model and
> then copy those parts into a tempetory buffer. This buffer is finally
> written to the ISP with correct length.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>  .../staging/media/atomisp/pci/sh_css_params.c | 37 ++++++++++++++++++-
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/sh_css_params.c b/drivers/staging/media/atomisp/pci/sh_css_params.c
> index 588f2adab058..2913d9d6d226 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css_params.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css_params.c
> @@ -3720,10 +3720,43 @@ struct ia_css_shading_table *ia_css_get_shading_table(struct ia_css_stream
>  
>  ia_css_ptr sh_css_store_sp_group_to_ddr(void)
>  {
> +	u8 *write_buf;
> +	u8 *buf_ptr;
> +
>  	IA_CSS_ENTER_LEAVE_PRIVATE("void");
> +
> +	write_buf = kzalloc(sizeof(u8) * 8192, GFP_KERNEL);

Please use sizeof(struct sh_css_sp_group) here, since you have dropped all the #ifdef-s in the header now that is the largest size which you need now.

> +
> +	buf_ptr = write_buf;
> +	if (IS_ISP2401) {
> +		memcpy(buf_ptr, &sh_css_sp_group.config, sizeof(u8) * 5);

This is wrong, there is a big hole between:

        u8                      no_isp_sync; /* Signal host immediately after start */
        u8                      enable_raw_pool_locking; /** Enable Raw Buffer Locking for HAL
        u8                      lock_all;

and:

        u8                 enable_isys_event_queue;
        u8                      disable_cont_vf;

filled with ISP2400 specific data now, so you are copying what likely is some empty alignment bytes before the ISP2400 specific input_formatter struct now instead of copying enable_isys_event_queue
and disable_cont_vf.

So you need to split the memcpy into 2 memcpy calls. You can calculate the source offset of enable_isys_event_queue in sh_css_sp_group.config with offsetof(struct sh_css_sp_config, enable_isys_event_queue), or better yet, just take the address of it:

	if (IS_ISP2401) {
		memcpy(buf_ptr, &sh_css_sp_group.config, 3);
		buf_ptr += 3;
		memcpy(buf_ptr, &sh_css_sp_group.config.enable_isys_event_queue, 2);
		buf_ptr += 2;
		memset(buf_ptr, 0, 3);
		buf_ptr += 3; /* Padding 3 bytes for struct sh_css_sp_config*/
	} else {

Also note I have dropped the "* sizeof(u8)" here, you already dropped that yourself for the memset / padding bits and dropping it makes the code easier to read IMHO.
		


> +		buf_ptr += (sizeof(u8) * 5);
> +		memset(buf_ptr, 0, 3);
> +		buf_ptr += 3; /* Padding 3 bytes for struct sh_css_sp_config*/
> +	} else {
> +		memcpy(buf_ptr, &sh_css_sp_group.config, sizeof(sh_css_sp_group.config));
> +		buf_ptr += sizeof(sh_css_sp_group.config);
> +	}
> +
> +	memcpy(buf_ptr, &sh_css_sp_group.pipe, sizeof(sh_css_sp_group.pipe));
> +	buf_ptr += sizeof(sh_css_sp_group.pipe);
> +
> +	if (IS_ISP2401) {
> +		memcpy(buf_ptr, &sh_css_sp_group.pipe_io, sizeof(sh_css_sp_group.pipe_io));
> +		buf_ptr += sizeof(sh_css_sp_group.pipe_io);
> +		memcpy(buf_ptr, &sh_css_sp_group.pipe_io_status,
> +		       sizeof(sh_css_sp_group.pipe_io_status));
> +		buf_ptr += sizeof(sh_css_sp_group.pipe_io_status);
> +	}
> +
> +	memcpy(buf_ptr, &sh_css_sp_group.debug, sizeof(sh_css_sp_group.debug));
> +	buf_ptr += sizeof(sh_css_sp_group.debug);
> +
>  	hmm_store(xmem_sp_group_ptrs,
> -		   &sh_css_sp_group,
> -		   sizeof(struct sh_css_sp_group));
> +		  write_buf,
> +		  buf_ptr - write_buf);
> +
> +	kfree(write_buf);
>  	return xmem_sp_group_ptrs;
>  }
>  

The rest looks good at a quick glance, but I need to take a closer look later.

Regards,

Hans
Hans de Goede June 6, 2023, 1:19 p.m. UTC | #4 
Hi Kate,

On 6/6/23 13:02, Hans de Goede wrote:
> Hi Kate,
> 
> On 6/5/23 12:29, Kate Hsuan wrote:
>> Pick up the necessary part of sp_group configuration for both model and
>> then copy those parts into a tempetory buffer. This buffer is finally
>> written to the ISP with correct length.
>>
>> Signed-off-by: Kate Hsuan <hpa@redhat.com>
>> ---
>>  .../staging/media/atomisp/pci/sh_css_params.c | 37 ++++++++++++++++++-
>>  1 file changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/atomisp/pci/sh_css_params.c b/drivers/staging/media/atomisp/pci/sh_css_params.c
>> index 588f2adab058..2913d9d6d226 100644
>> --- a/drivers/staging/media/atomisp/pci/sh_css_params.c
>> +++ b/drivers/staging/media/atomisp/pci/sh_css_params.c
>> @@ -3720,10 +3720,43 @@ struct ia_css_shading_table *ia_css_get_shading_table(struct ia_css_stream
>>  
>>  ia_css_ptr sh_css_store_sp_group_to_ddr(void)
>>  {
>> +	u8 *write_buf;
>> +	u8 *buf_ptr;
>> +
>>  	IA_CSS_ENTER_LEAVE_PRIVATE("void");
>> +
>> +	write_buf = kzalloc(sizeof(u8) * 8192, GFP_KERNEL);
> 
> Please use sizeof(struct sh_css_sp_group) here, since you have dropped all the #ifdef-s in the header now that is the largest size which you need now.
> 
>> +
>> +	buf_ptr = write_buf;
>> +	if (IS_ISP2401) {
>> +		memcpy(buf_ptr, &sh_css_sp_group.config, sizeof(u8) * 5);
> 
> This is wrong, there is a big hole between:
> 
>         u8                      no_isp_sync; /* Signal host immediately after start */
>         u8                      enable_raw_pool_locking; /** Enable Raw Buffer Locking for HAL
>         u8                      lock_all;
> 
> and:
> 
>         u8                 enable_isys_event_queue;
>         u8                      disable_cont_vf;
> 
> filled with ISP2400 specific data now, so you are copying what likely is some empty alignment bytes before the ISP2400 specific input_formatter struct now instead of copying enable_isys_event_queue
> and disable_cont_vf.
> 
> So you need to split the memcpy into 2 memcpy calls. You can calculate the source offset of enable_isys_event_queue in sh_css_sp_group.config with offsetof(struct sh_css_sp_config, enable_isys_event_queue), or better yet, just take the address of it:
> 
> 	if (IS_ISP2401) {
> 		memcpy(buf_ptr, &sh_css_sp_group.config, 3);
> 		buf_ptr += 3;
> 		memcpy(buf_ptr, &sh_css_sp_group.config.enable_isys_event_queue, 2);
> 		buf_ptr += 2;
> 		memset(buf_ptr, 0, 3);
> 		buf_ptr += 3; /* Padding 3 bytes for struct sh_css_sp_config*/
> 	} else {
> 
> Also note I have dropped the "* sizeof(u8)" here, you already dropped that yourself for the memset / padding bits and dropping it makes the code easier to read IMHO.
> 		
> 
> 
>> +		buf_ptr += (sizeof(u8) * 5);
>> +		memset(buf_ptr, 0, 3);
>> +		buf_ptr += 3; /* Padding 3 bytes for struct sh_css_sp_config*/
>> +	} else {
>> +		memcpy(buf_ptr, &sh_css_sp_group.config, sizeof(sh_css_sp_group.config));
>> +		buf_ptr += sizeof(sh_css_sp_group.config);
>> +	}
>> +
>> +	memcpy(buf_ptr, &sh_css_sp_group.pipe, sizeof(sh_css_sp_group.pipe));
>> +	buf_ptr += sizeof(sh_css_sp_group.pipe);
>> +
>> +	if (IS_ISP2401) {
>> +		memcpy(buf_ptr, &sh_css_sp_group.pipe_io, sizeof(sh_css_sp_group.pipe_io));
>> +		buf_ptr += sizeof(sh_css_sp_group.pipe_io);
>> +		memcpy(buf_ptr, &sh_css_sp_group.pipe_io_status,
>> +		       sizeof(sh_css_sp_group.pipe_io_status));
>> +		buf_ptr += sizeof(sh_css_sp_group.pipe_io_status);
>> +	}
>> +
>> +	memcpy(buf_ptr, &sh_css_sp_group.debug, sizeof(sh_css_sp_group.debug));
>> +	buf_ptr += sizeof(sh_css_sp_group.debug);
>> +
>>  	hmm_store(xmem_sp_group_ptrs,
>> -		   &sh_css_sp_group,
>> -		   sizeof(struct sh_css_sp_group));
>> +		  write_buf,
>> +		  buf_ptr - write_buf);
>> +
>> +	kfree(write_buf);
>>  	return xmem_sp_group_ptrs;
>>  }
>>  
> 
> The rest looks good at a quick glance, but I need to take a closer look later.

I have taken a closer look at the rest of the patch now and except for my one previous remark this looks good to me.

Thank you for working on this.

Regards,

Hans
Kate Hsuan June 7, 2023, 3:20 a.m. UTC | #5 
Hi,

On Tue, Jun 6, 2023 at 7:02 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Kate,
>
> On 6/5/23 12:29, Kate Hsuan wrote:
> > Pick up the necessary part of sp_group configuration for both model and
> > then copy those parts into a tempetory buffer. This buffer is finally
> > written to the ISP with correct length.
> >
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
> > ---
> >  .../staging/media/atomisp/pci/sh_css_params.c | 37 ++++++++++++++++++-
> >  1 file changed, 35 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/sh_css_params.c b/drivers/staging/media/atomisp/pci/sh_css_params.c
> > index 588f2adab058..2913d9d6d226 100644
> > --- a/drivers/staging/media/atomisp/pci/sh_css_params.c
> > +++ b/drivers/staging/media/atomisp/pci/sh_css_params.c
> > @@ -3720,10 +3720,43 @@ struct ia_css_shading_table *ia_css_get_shading_table(struct ia_css_stream
> >
> >  ia_css_ptr sh_css_store_sp_group_to_ddr(void)
> >  {
> > +     u8 *write_buf;
> > +     u8 *buf_ptr;
> > +
> >       IA_CSS_ENTER_LEAVE_PRIVATE("void");
> > +
> > +     write_buf = kzalloc(sizeof(u8) * 8192, GFP_KERNEL);
>
> Please use sizeof(struct sh_css_sp_group) here, since you have dropped all the #ifdef-s in the header now that is the largest size which you need now.

Okay.

>
> > +
> > +     buf_ptr = write_buf;
> > +     if (IS_ISP2401) {
> > +             memcpy(buf_ptr, &sh_css_sp_group.config, sizeof(u8) * 5);
>
> This is wrong, there is a big hole between:
>
>         u8                      no_isp_sync; /* Signal host immediately after start */
>         u8                      enable_raw_pool_locking; /** Enable Raw Buffer Locking for HAL
>         u8                      lock_all;
>
> and:
>
>         u8                 enable_isys_event_queue;
>         u8                      disable_cont_vf;
>
> filled with ISP2400 specific data now, so you are copying what likely is some empty alignment bytes before the ISP2400 specific input_formatter struct now instead of copying enable_isys_event_queue
> and disable_cont_vf.
>
> So you need to split the memcpy into 2 memcpy calls. You can calculate the source offset of enable_isys_event_queue in sh_css_sp_group.config with offsetof(struct sh_css_sp_config, enable_isys_event_queue), or better yet, just take the address of it:
>
>         if (IS_ISP2401) {
>                 memcpy(buf_ptr, &sh_css_sp_group.config, 3);
>                 buf_ptr += 3;
>                 memcpy(buf_ptr, &sh_css_sp_group.config.enable_isys_event_queue, 2);
>                 buf_ptr += 2;
>                 memset(buf_ptr, 0, 3);
>                 buf_ptr += 3; /* Padding 3 bytes for struct sh_css_sp_config*/
>         } else {
>
> Also note I have dropped the "* sizeof(u8)" here, you already dropped that yourself for the memset / padding bits and dropping it makes the code easier to read IMHO.
>
>

Ops, My bad. I tried to simplify the memcpy() part and made the things
wrong. I should skip the structure between them. I'll fix it in the v2
patch.

Thank you for pointing out this error.

>
> > +             buf_ptr += (sizeof(u8) * 5);
> > +             memset(buf_ptr, 0, 3);
> > +             buf_ptr += 3; /* Padding 3 bytes for struct sh_css_sp_config*/
> > +     } else {
> > +             memcpy(buf_ptr, &sh_css_sp_group.config, sizeof(sh_css_sp_group.config));
> > +             buf_ptr += sizeof(sh_css_sp_group.config);
> > +     }
> > +
> > +     memcpy(buf_ptr, &sh_css_sp_group.pipe, sizeof(sh_css_sp_group.pipe));
> > +     buf_ptr += sizeof(sh_css_sp_group.pipe);
> > +
> > +     if (IS_ISP2401) {
> > +             memcpy(buf_ptr, &sh_css_sp_group.pipe_io, sizeof(sh_css_sp_group.pipe_io));
> > +             buf_ptr += sizeof(sh_css_sp_group.pipe_io);
> > +             memcpy(buf_ptr, &sh_css_sp_group.pipe_io_status,
> > +                    sizeof(sh_css_sp_group.pipe_io_status));
> > +             buf_ptr += sizeof(sh_css_sp_group.pipe_io_status);
> > +     }
> > +
> > +     memcpy(buf_ptr, &sh_css_sp_group.debug, sizeof(sh_css_sp_group.debug));
> > +     buf_ptr += sizeof(sh_css_sp_group.debug);
> > +
> >       hmm_store(xmem_sp_group_ptrs,
> > -                &sh_css_sp_group,
> > -                sizeof(struct sh_css_sp_group));
> > +               write_buf,
> > +               buf_ptr - write_buf);
> > +
> > +     kfree(write_buf);
> >       return xmem_sp_group_ptrs;
> >  }
> >
>
> The rest looks good at a quick glance, but I need to take a closer look later.
>
> Regards,
>
> Hans
>
diff mbox series

Patch

diff --git a/drivers/staging/media/atomisp/pci/sh_css_params.c b/drivers/staging/media/atomisp/pci/sh_css_params.c
index 588f2adab058..2913d9d6d226 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_params.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_params.c
@@ -3720,10 +3720,43 @@  struct ia_css_shading_table *ia_css_get_shading_table(struct ia_css_stream
 
 ia_css_ptr sh_css_store_sp_group_to_ddr(void)
 {
+	u8 *write_buf;
+	u8 *buf_ptr;
+
 	IA_CSS_ENTER_LEAVE_PRIVATE("void");
+
+	write_buf = kzalloc(sizeof(u8) * 8192, GFP_KERNEL);
+
+	buf_ptr = write_buf;
+	if (IS_ISP2401) {
+		memcpy(buf_ptr, &sh_css_sp_group.config, sizeof(u8) * 5);
+		buf_ptr += (sizeof(u8) * 5);
+		memset(buf_ptr, 0, 3);
+		buf_ptr += 3; /* Padding 3 bytes for struct sh_css_sp_config*/
+	} else {
+		memcpy(buf_ptr, &sh_css_sp_group.config, sizeof(sh_css_sp_group.config));
+		buf_ptr += sizeof(sh_css_sp_group.config);
+	}
+
+	memcpy(buf_ptr, &sh_css_sp_group.pipe, sizeof(sh_css_sp_group.pipe));
+	buf_ptr += sizeof(sh_css_sp_group.pipe);
+
+	if (IS_ISP2401) {
+		memcpy(buf_ptr, &sh_css_sp_group.pipe_io, sizeof(sh_css_sp_group.pipe_io));
+		buf_ptr += sizeof(sh_css_sp_group.pipe_io);
+		memcpy(buf_ptr, &sh_css_sp_group.pipe_io_status,
+		       sizeof(sh_css_sp_group.pipe_io_status));
+		buf_ptr += sizeof(sh_css_sp_group.pipe_io_status);
+	}
+
+	memcpy(buf_ptr, &sh_css_sp_group.debug, sizeof(sh_css_sp_group.debug));
+	buf_ptr += sizeof(sh_css_sp_group.debug);
+
 	hmm_store(xmem_sp_group_ptrs,
-		   &sh_css_sp_group,
-		   sizeof(struct sh_css_sp_group));
+		  write_buf,
+		  buf_ptr - write_buf);
+
+	kfree(write_buf);
 	return xmem_sp_group_ptrs;
 }