Re: [PATCH v6 4/5] drm/bridge: anx7625: add HDCP support

2021-03-29 Thread Sean Paul 
On Mon, Mar 29, 2021 at 6:27 AM Xin Ji  wrote:
>
> On Thu, Mar 25, 2021 at 02:19:23PM -0400, Sean Paul wrote:
> > On Fri, Mar 19, 2021 at 2:35 AM Xin Ji  wrote:
> > >
> > > Add HDCP feature, enable HDCP function through chip internal key
> > > and downstream's capability.
> > >
> > > Signed-off-by: Xin Ji 
> > > ---

/snip

> > >  static void anx7625_dp_start(struct anx7625_data *ctx)
> > >  {
> > > int ret;
> > > @@ -643,6 +787,9 @@ static void anx7625_dp_start(struct anx7625_data *ctx)
> > > return;
> > > }
> > >
> > > +   /* HDCP config */
> > > +   anx7625_hdcp_setting(ctx);
> >
> > You should really use the "Content Protection" property to
> > enable/disable HDCP instead of force-enabling it at all times.
> >
> > Sean
> Hi Sean, it's hard to implement "Content Protection" property, we have
> implemented HDCP in firmware, it is not compatible with it. We don't
> have interface to get Downstream Cert.
> Thanks,
> Xin

Hi Xin,
I'm sorry, I don't understand what you mean when you say you don't
have an interface to get Downstream Cert.

The Content Protection property is just a means through which
userspace can turn on and turn off HDCP when it needs. As far as I can
tell, your patch turns on HDCP when the display is enabled and leaves
it on until it is disabled. This is undesirable since it forces HDCP
on the user.

Is it impossible to enable/disable HDCP outside of display
enable/disable on your hardware?

Thanks,

Sean

> >
> > > +
> > > if (ctx->pdata.is_dpi)
> > > ret = anx7625_dpi_config(ctx);
> > > else

/snip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v6 4/5] drm/bridge: anx7625: add HDCP support

2021-03-25 Thread Sean Paul 
On Fri, Mar 19, 2021 at 2:35 AM Xin Ji  wrote:
>
> Add HDCP feature, enable HDCP function through chip internal key
> and downstream's capability.
>
> Signed-off-by: Xin Ji 
> ---
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 147 ++
>  drivers/gpu/drm/bridge/analogix/anx7625.h |  36 ++
>  2 files changed, 183 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c 
> b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 8c514b46d361..b424a570effa 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -633,6 +633,150 @@ static int anx7625_dpi_config(struct anx7625_data *ctx)
> return ret;
>  }
>
> +static int anx7625_aux_dpcd_read(struct anx7625_data *ctx,
> +u8 addrh, u8 addrm, u8 addrl,
> +u8 len, u8 *buf)
> +{
> +   struct device *dev = >client->dev;
> +   int ret;
> +   u8 cmd;
> +
> +   if (len > MAX_DPCD_BUFFER_SIZE) {
> +   DRM_DEV_ERROR(dev, "exceed aux buffer len.\n");
> +   return -E2BIG;
> +   }
> +
> +   cmd = ((len - 1) << 4) | 0x09;
> +
> +   /* Set command and length */
> +   ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +   AP_AUX_COMMAND, cmd);
> +
> +   /* Set aux access address */
> +   ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +AP_AUX_ADDR_7_0, addrl);
> +   ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +AP_AUX_ADDR_15_8, addrm);
> +   ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +AP_AUX_ADDR_19_16, addrh);
> +
> +   /* Enable aux access */
> +   ret |= anx7625_write_or(ctx, ctx->i2c.rx_p0_client,
> +   AP_AUX_CTRL_STATUS, AP_AUX_CTRL_OP_EN);
> +
> +   if (ret < 0) {
> +   DRM_DEV_ERROR(dev, "cannot access aux related register.\n");
> +   return -EIO;
> +   }
> +
> +   usleep_range(2000, 2100);
> +
> +   ret = wait_aux_op_finish(ctx);
> +   if (ret) {
> +   DRM_DEV_ERROR(dev, "aux IO error: wait aux op finish.\n");
> +   return ret;
> +   }
> +
> +   ret = anx7625_reg_block_read(ctx, ctx->i2c.rx_p0_client,
> +AP_AUX_BUFF_START, len, buf);
> +   if (ret < 0) {
> +   DRM_DEV_ERROR(dev, "read dpcd register failed\n");
> +   return -EIO;
> +   }
> +
> +   return 0;
> +}
> +
> +static int anx7625_read_flash_status(struct anx7625_data *ctx)
> +{
> +   return anx7625_reg_read(ctx, ctx->i2c.rx_p0_client, R_RAM_CTRL);
> +}
> +
> +static int anx7625_hdcp_key_probe(struct anx7625_data *ctx)
> +{
> +   int ret, val;
> +   struct device *dev = >client->dev;
> +   u8 ident[32];
> +
> +   ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +   FLASH_ADDR_HIGH, 0x91);
> +   ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +FLASH_ADDR_LOW, 0xA0);
> +   if (ret < 0) {
> +   DRM_DEV_ERROR(dev, "IO error : set key flash address.\n");
> +   return ret;
> +   }
> +
> +   ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +   FLASH_LEN_HIGH, (FLASH_BUF_LEN - 1) >> 8);
> +   ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +FLASH_LEN_LOW, (FLASH_BUF_LEN - 1) & 0xFF);
> +   if (ret < 0) {
> +   DRM_DEV_ERROR(dev, "IO error : set key flash len.\n");
> +   return ret;
> +   }
> +
> +   ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +   R_FLASH_RW_CTRL, FLASH_READ);
> +   ret |= readx_poll_timeout(anx7625_read_flash_status,
> + ctx, val,
> + ((val & FLASH_DONE) || (val < 0)),
> + 2000,
> + 2000 * 150);
> +   if (ret) {
> +   DRM_DEV_ERROR(dev, "flash read access fail!\n");
> +   return -EIO;
> +   }
> +
> +   ret = anx7625_reg_block_read(ctx, ctx->i2c.rx_p0_client,
> +FLASH_BUF_BASE_ADDR,
> +FLASH_BUF_LEN, ident);
> +   if (ret < 0) {
> +   DRM_DEV_ERROR(dev, "read flash data fail!\n");
> +   return -EIO;
> +   }
> +
> +   if (ident[29] == 0xFF && ident[30] == 0xFF && ident[31] == 0xFF)
> +   return -EINVAL;
> +
> +   return 0;
> +}
> +
> +static int anx7625_hdcp_setting(struct anx7625_data *ctx)
> +{
> +   u8 bcap;
> +   int ret;
> +   struct device *dev = >client->dev;
> +
> +   ret = anx7625_hdcp_key_probe(ctx);
> +   if (ret) {
> +

Re: [Outreachy kernel] [PATCH v2] Staging: ccree: Remove unused variable monitor_lock

2017-09-11 Thread Sean Paul 
On Mon, Sep 11, 2017 at 12:28 PM, Srishti Sharma <srishtis...@gmail.com> wrote:
> Remove the variable monitor_lock as it is not used anywhere.
>
> Signed-off-by: Srishti Sharma <srishtis...@gmail.com>

Reviewed-by: Sean Paul <seanp...@chromium.org>

> ---
> Changes in v2:
>  -The variable that was not to be declared as volatile can be
>   eliminated as it is not being used anywhere.
>
>  drivers/staging/ccree/ssi_request_mgr.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/staging/ccree/ssi_request_mgr.c 
> b/drivers/staging/ccree/ssi_request_mgr.c
> index e5c2f92..b94a91f 100644
> --- a/drivers/staging/ccree/ssi_request_mgr.c
> +++ b/drivers/staging/ccree/ssi_request_mgr.c
> @@ -49,7 +49,6 @@ struct ssi_request_mgr_handle {
> dma_addr_t dummy_comp_buff_dma;
> struct cc_hw_desc monitor_desc;
>
> -   volatile unsigned long monitor_lock;
>  #ifdef COMP_IN_WQ
> struct workqueue_struct *workq;
> struct delayed_work compwork;
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups 
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/1505147317-13411-1-git-send-email-srishtishar%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [Outreachy kernel] Re: [PATCH] Staging: ccree: Don't use volatile for monitor_lock

2017-09-11 Thread Sean Paul 
On Mon, Sep 11, 2017 at 12:08 PM, Srishti Sharma  wrote:
> On Mon, Sep 11, 2017 at 9:34 PM, Greg KH  wrote:
>> On Mon, Sep 11, 2017 at 09:29:31PM +0530, Srishti Sharma wrote:
>>> The use of volatile for the variable monitor_lock is unnecessary.
>>>
>>> Signed-off-by: Srishti Sharma 
>>> ---
>>>  drivers/staging/ccree/ssi_request_mgr.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/ccree/ssi_request_mgr.c 
>>> b/drivers/staging/ccree/ssi_request_mgr.c
>>> index e5c2f92..7d77941 100644
>>> --- a/drivers/staging/ccree/ssi_request_mgr.c
>>> +++ b/drivers/staging/ccree/ssi_request_mgr.c
>>> @@ -49,7 +49,7 @@ struct ssi_request_mgr_handle {
>>>   dma_addr_t dummy_comp_buff_dma;
>>>   struct cc_hw_desc monitor_desc;
>>>
>>> - volatile unsigned long monitor_lock;
>>> + unsigned long monitor_lock;
>>
>> While volatile is not right, odds are, this is still totally wrong as
>> well.  How about using a "real" lock instead?
>
> I tried to find where is this variable being used in the code, but I
> didn't find any usage of it . It might be an important attribute of
> this structure definition but, I don't see it's value being set to
> anything or being used somewhere .
>

AFAICT, it's not used. Your patch should just remove it instead :)

Sean

> Regards,
> Srishti
>>
>> thanks,
>>
>> greg k-h
>
> --
> You received this message because you are subscribed to the Google Groups 
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/CAB3L5oxcyhgyy8EuGuPo9QtJQd-W7JTgQQE1PfopZFmSx58P9g%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [Outreachy kernel] [PATCH] Staging: media: imx: Prefer using BIT macro

2017-09-08 Thread Sean Paul 
On Fri, Sep 8, 2017 at 11:11 AM, Srishti Sharma  wrote:
> Use BIT(x) instead of (1<
> Signed-off-by: Srishti Sharma 
> ---
>  drivers/staging/media/imx/imx-media.h | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media.h 
> b/drivers/staging/media/imx/imx-media.h
> index d409170..e5b8d29 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -310,16 +310,16 @@ void imx_media_capture_device_set_format(struct 
> imx_media_video_dev *vdev,
>  void imx_media_capture_device_error(struct imx_media_video_dev *vdev);
>
>  /* subdev group ids */
> -#define IMX_MEDIA_GRP_ID_SENSOR(1 << 8)
> -#define IMX_MEDIA_GRP_ID_VIDMUX(1 << 9)
> -#define IMX_MEDIA_GRP_ID_CSI2  (1 << 10)
> +#define IMX_MEDIA_GRP_ID_SENSORBIT(8)
> +#define IMX_MEDIA_GRP_ID_VIDMUXBIT(9)
> +#define IMX_MEDIA_GRP_ID_CSI2  BIT(10)
>  #define IMX_MEDIA_GRP_ID_CSI_BIT   11
>  #define IMX_MEDIA_GRP_ID_CSI   (0x3 << IMX_MEDIA_GRP_ID_CSI_BIT)
> -#define IMX_MEDIA_GRP_ID_CSI0  (1 << IMX_MEDIA_GRP_ID_CSI_BIT)
> +#define IMX_MEDIA_GRP_ID_CSI0  BIT(IMX_MEDIA_GRP_ID_CSI_BIT)
>  #define IMX_MEDIA_GRP_ID_CSI1  (2 << IMX_MEDIA_GRP_ID_CSI_BIT)
> -#define IMX_MEDIA_GRP_ID_VDIC  (1 << 13)
> -#define IMX_MEDIA_GRP_ID_IC_PRP(1 << 14)
> -#define IMX_MEDIA_GRP_ID_IC_PRPENC (1 << 15)
> -#define IMX_MEDIA_GRP_ID_IC_PRPVF  (1 << 16)
> +#define IMX_MEDIA_GRP_ID_VDIC  BIT(13)
> +#define IMX_MEDIA_GRP_ID_IC_PRPBIT(14)
> +#define IMX_MEDIA_GRP_ID_IC_PRPENC BIT(15)
> +#define IMX_MEDIA_GRP_ID_IC_PRPVF  BIT(16)

Hi Srishti,
Thanks for your patch.

Perhaps this is just personal preference, but I find the previous
version more readable. Since IMX_MEDIA_GRP_ID_CSI and
IMX_MEDIA_GRP_ID_CSI1 are multi-bit fields, you can't fully eliminate
the bit shift operations, so you end up with a mix, which is kind of
ugly.

Sean

>
>  #endif
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups 
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/1504883469-8127-1-git-send-email-srishtishar%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 28/28] drm: vboxvideo: switch to drm_*_get(), drm_*_put() helpers

2017-08-11 Thread Sean Paul 
On Fri, Aug 11, 2017 at 12:11 PM, Hans de Goede <hdego...@redhat.com> wrote:
> Hi,
>
> On 11-08-17 18:04, Sean Paul wrote:
>>
>> On Fri, Aug 11, 2017 at 03:26:45PM +0200, Hans de Goede wrote:
>>>
>>> Hi,
>>>
>>> On 11-08-17 14:33, Cihangir Akturk wrote:
>>>>
>>>> Use drm_*_get() and drm_*_put() helpers instead of drm_*_reference()
>>>> and drm_*_unreference() helpers.
>>>>
>>>> drm_*_reference() and drm_*_unreference() functions are just
>>>> compatibility alias for drm_*_get() and drm_*_put() and should not be
>>>> used by new code. So convert all users of compatibility functions to
>>>> use the new APIs.
>>>>
>>>> Generated by: scripts/coccinelle/api/drm-get-put.cocci
>>>>
>>>> Signed-off-by: Cihangir Akturk <cakt...@gmail.com>
>>>
>>>
>>> Thank you for doing this, looks good to me:
>>>
>>> Reviewed-by: Hans de Goede <hdego...@redhat.com>
>>>
>>
>> Applied to drm-misc-next, thank you for the review!
>
>
> Erm vboxvideo is in staging, does this mean all patches for
> it will now go through drm-misc-next despite it being in
> staging (*) ?  Because if some patches get merged through
> drm-misc-next and some through Greg's staging repo that
> is not going to end well.
>

Hi Hans,
Thanks for pointing this out. I picked it up as I was vacuuming up the
rest of the set that was not yet applied, not realizing the staging
implications. It's not my intention to start taking vboxvideo through
-misc.

Hopefully this won't cause any nasty conflicts with Greg's staging
tree, and we can treat it as a one-off. If it does cause problems, I
can revert it in -misc in favor of taking it through staging.

Sean

> Regards,
>
> Hans
>
>
>
> *) that is fine, the same is done for e.g. the media drivers afaik
>
>
>
>>
>> Sean
>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>
>>>> ---
>>>>drivers/staging/vboxvideo/vbox_fb.c   | 2 +-
>>>>drivers/staging/vboxvideo/vbox_main.c | 8 
>>>>drivers/staging/vboxvideo/vbox_mode.c | 2 +-
>>>>3 files changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/vboxvideo/vbox_fb.c
>>>> b/drivers/staging/vboxvideo/vbox_fb.c
>>>> index bf66358..c157284 100644
>>>> --- a/drivers/staging/vboxvideo/vbox_fb.c
>>>> +++ b/drivers/staging/vboxvideo/vbox_fb.c
>>>> @@ -343,7 +343,7 @@ void vbox_fbdev_fini(struct drm_device *dev)
>>>> vbox_bo_unpin(bo);
>>>> vbox_bo_unreserve(bo);
>>>> }
>>>> -   drm_gem_object_unreference_unlocked(afb->obj);
>>>> +   drm_gem_object_put_unlocked(afb->obj);
>>>> afb->obj = NULL;
>>>> }
>>>> drm_fb_helper_fini(>helper);
>>>> diff --git a/drivers/staging/vboxvideo/vbox_main.c
>>>> b/drivers/staging/vboxvideo/vbox_main.c
>>>> index d0c6ec7..80bd039 100644
>>>> --- a/drivers/staging/vboxvideo/vbox_main.c
>>>> +++ b/drivers/staging/vboxvideo/vbox_main.c
>>>> @@ -40,7 +40,7 @@ static void vbox_user_framebuffer_destroy(struct
>>>> drm_framebuffer *fb)
>>>> struct vbox_framebuffer *vbox_fb = to_vbox_framebuffer(fb);
>>>> if (vbox_fb->obj)
>>>> -   drm_gem_object_unreference_unlocked(vbox_fb->obj);
>>>> +   drm_gem_object_put_unlocked(vbox_fb->obj);
>>>> drm_framebuffer_cleanup(fb);
>>>> kfree(fb);
>>>> @@ -198,7 +198,7 @@ static struct drm_framebuffer
>>>> *vbox_user_framebuffer_create(
>>>>err_free_vbox_fb:
>>>> kfree(vbox_fb);
>>>>err_unref_obj:
>>>> -   drm_gem_object_unreference_unlocked(obj);
>>>> +   drm_gem_object_put_unlocked(obj);
>>>> return ERR_PTR(ret);
>>>>}
>>>> @@ -472,7 +472,7 @@ int vbox_dumb_create(struct drm_file *file,
>>>> return ret;
>>>> ret = drm_gem_handle_create(file, gobj, );
>>>> -   drm_gem_object_unreference_unlocked(gobj);
>>>> +   drm_gem_object_put_unlocked(gobj);
>>>> if (ret)
>>>> return ret;
>>

Re: [PATCH v3 28/28] drm: vboxvideo: switch to drm_*_get(), drm_*_put() helpers

2017-08-11 Thread Sean Paul 
On Fri, Aug 11, 2017 at 03:26:45PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 11-08-17 14:33, Cihangir Akturk wrote:
> > Use drm_*_get() and drm_*_put() helpers instead of drm_*_reference()
> > and drm_*_unreference() helpers.
> > 
> > drm_*_reference() and drm_*_unreference() functions are just
> > compatibility alias for drm_*_get() and drm_*_put() and should not be
> > used by new code. So convert all users of compatibility functions to
> > use the new APIs.
> > 
> > Generated by: scripts/coccinelle/api/drm-get-put.cocci
> > 
> > Signed-off-by: Cihangir Akturk <cakt...@gmail.com>
> 
> Thank you for doing this, looks good to me:
> 
> Reviewed-by: Hans de Goede <hdego...@redhat.com>
> 

Applied to drm-misc-next, thank you for the review!

Sean

> Regards,
> 
> Hans
> 
> 
> 
> > ---
> >   drivers/staging/vboxvideo/vbox_fb.c   | 2 +-
> >   drivers/staging/vboxvideo/vbox_main.c | 8 
> >   drivers/staging/vboxvideo/vbox_mode.c | 2 +-
> >   3 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/vboxvideo/vbox_fb.c 
> > b/drivers/staging/vboxvideo/vbox_fb.c
> > index bf66358..c157284 100644
> > --- a/drivers/staging/vboxvideo/vbox_fb.c
> > +++ b/drivers/staging/vboxvideo/vbox_fb.c
> > @@ -343,7 +343,7 @@ void vbox_fbdev_fini(struct drm_device *dev)
> > vbox_bo_unpin(bo);
> > vbox_bo_unreserve(bo);
> > }
> > -   drm_gem_object_unreference_unlocked(afb->obj);
> > +   drm_gem_object_put_unlocked(afb->obj);
> > afb->obj = NULL;
> > }
> > drm_fb_helper_fini(>helper);
> > diff --git a/drivers/staging/vboxvideo/vbox_main.c 
> > b/drivers/staging/vboxvideo/vbox_main.c
> > index d0c6ec7..80bd039 100644
> > --- a/drivers/staging/vboxvideo/vbox_main.c
> > +++ b/drivers/staging/vboxvideo/vbox_main.c
> > @@ -40,7 +40,7 @@ static void vbox_user_framebuffer_destroy(struct 
> > drm_framebuffer *fb)
> > struct vbox_framebuffer *vbox_fb = to_vbox_framebuffer(fb);
> > if (vbox_fb->obj)
> > -   drm_gem_object_unreference_unlocked(vbox_fb->obj);
> > +   drm_gem_object_put_unlocked(vbox_fb->obj);
> > drm_framebuffer_cleanup(fb);
> > kfree(fb);
> > @@ -198,7 +198,7 @@ static struct drm_framebuffer 
> > *vbox_user_framebuffer_create(
> >   err_free_vbox_fb:
> > kfree(vbox_fb);
> >   err_unref_obj:
> > -   drm_gem_object_unreference_unlocked(obj);
> > +   drm_gem_object_put_unlocked(obj);
> > return ERR_PTR(ret);
> >   }
> > @@ -472,7 +472,7 @@ int vbox_dumb_create(struct drm_file *file,
> > return ret;
> > ret = drm_gem_handle_create(file, gobj, );
> > -   drm_gem_object_unreference_unlocked(gobj);
> > +   drm_gem_object_put_unlocked(gobj);
> > if (ret)
> > return ret;
> > @@ -525,7 +525,7 @@ vbox_dumb_mmap_offset(struct drm_file *file,
> > bo = gem_to_vbox_bo(obj);
> > *offset = vbox_bo_mmap_offset(bo);
> > -   drm_gem_object_unreference(obj);
> > +   drm_gem_object_put(obj);
> > ret = 0;
> >   out_unlock:
> > diff --git a/drivers/staging/vboxvideo/vbox_mode.c 
> > b/drivers/staging/vboxvideo/vbox_mode.c
> > index 996da1c..e5b6383 100644
> > --- a/drivers/staging/vboxvideo/vbox_mode.c
> > +++ b/drivers/staging/vboxvideo/vbox_mode.c
> > @@ -812,7 +812,7 @@ static int vbox_cursor_set2(struct drm_crtc *crtc, 
> > struct drm_file *file_priv,
> >   out_unreserve_bo:
> > vbox_bo_unreserve(bo);
> >   out_unref_obj:
> > -   drm_gem_object_unreference_unlocked(obj);
> > +   drm_gem_object_put_unlocked(obj);
> > return ret;
> >   }
> > 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

2017-06-14 Thread Sean Paul 
On Wed, Jun 14, 2017 at 11:34:10AM +0200, Michael Thayer wrote:
> Hello Sean,
> 
> 13.06.2017 20:03, Sean Paul wrote:
> [Discussion of vboxvideo driver clean-up.]
> 
> > First, thank you for your submission.
> 
> Thank you for your feedback.
> 
> [Discussion of the OS-independent code in the driver submission.]
> 
> > I took a quick skim through your driver, and there doesn't seem to be much
> > secret sauce there that will be tough to keep up-to-date across platforms.
> 
> I have two particular concerns there: first if we add new functionality
> (which we would do out of tree first) it will need porting over.
> Acceleration support is the most likely candidate.  And if someone does
> make fixes to that part of the code in the kernel tree they will also
> need porting over.  I agree, that concern is probably overblown, and
> best addressed by keeping that part of the code as close to our tree as
> possible while still meeting kernel standards (hence my question as to
> what that would be).

Once the code is upstream, it's going to be difficult to motivate developers to
keep the driver close to your downstream version. If I'm working on feature X
which touches your driver, I'm not going to figure out how your internal version
works so that I might ease your pain. I don't mean that to be rude, just
realistic.

Sean

> 
> The second concern is not relevant to DRI, but it concerns our other
> guest drivers (not the host one with the C++ in it Greg!) which Hans
> also expressed interest in putting upstream after seeing how vboxvideo
> fares.  The OS-independent part is quite a bit larger and more volatile,
> though it has thankfully stablised a lot.  That concern is probably also
> overblown, though I do wonder whether upstreaming those driver is the
> best solution (that would be Hans's call though).
> 
> > One other concern I have is that I noticed there are a few functions
> > declared/defined in the osindependent code that are never used outside of 
> > it, so
> > we have dead code off the hop.
> 
> If the OS-independent part gets converted then they would be removed in
> the process.  Thank you for the reminder.
> 
> [...]
> 
> > IMO, in order to accept the driver in drm, the osindependent code needs to 
> > be
> > stripped out and it needs to be converted to atomic. Whether you want to do
> > this out of tree, or in staging is up to you. As Dave mentioned, people 
> > often
> > overlook staging when making drm core changes, so you'll likely face the 
> > same
> > moving target issues either way.
> 
> Conversion to Atomic would probably have to happen at some time or
> another anyway.  I have put that off (out-of-tree) so far because I was
> tracking the AST driver as closely as possible as the simplest way of
> picking up fixes, and because Dave, who wrote that, knows much more
> about drm drivers than me.
> 
> [...]
> 
> Regards
> Michael
> -- 
> Michael Thayer | VirtualBox engineer
> ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt
> 
> ORACLE Deutschland B.V. & Co. KG
> Hauptverwaltung: Riesstraße 25, D-80992 München
> Registergericht: Amtsgericht München, HRA 95603
> 
> Komplementärin: ORACLE Deutschland Verwaltung B.V.
> Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister
> der Handelskammer Midden-Nederland, Nr. 30143697
> Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

2017-06-13 Thread Sean Paul 
 used outside of it, so
we have dead code off the hop.

> 
> Please be clear, I am not trying to dictate to anyone.  The code is GPL,
> it is fine to include it with whatever modifications you deem
> appropriate.  Since individual distributions are already doing that, it
> will still simplify our life somewhat if it is in the upstream kernel,
> in whatever form, rather than in multiple forks.
> 
> > Remember, this code needs us, we don't need this code at all :)
> 
> I assume that that was not meant that way, but that came over as
> slightly unfriendly to me.  I am assuming here that we are looking for
> the best way to do something which will be useful to a lot of people.
> 

IMO, in order to accept the driver in drm, the osindependent code needs to be
stripped out and it needs to be converted to atomic. Whether you want to do
this out of tree, or in staging is up to you. As Dave mentioned, people often
overlook staging when making drm core changes, so you'll likely face the same
moving target issues either way.

Sean


> Regards
> Michael
> 
> > good luck!
> > 
> > greg k-h
> > 
> -- 
> Michael Thayer | VirtualBox engineer
> ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt
> 
> ORACLE Deutschland B.V. & Co. KG
> Hauptverwaltung: Riesstraße 25, D-80992 München
> Registergericht: Amtsgericht München, HRA 95603
> 
> Komplementärin: ORACLE Deutschland Verwaltung B.V.
> Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister
> der Handelskammer Midden-Nederland, Nr. 30143697
> Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems

2014-01-08 Thread Sean Paul 
On Wed, Jan 8, 2014 at 4:36 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
 On Tue, Jan 07, 2014 at 03:18:21PM -0500, Sean Paul wrote:
 On Thu, Jan 2, 2014 at 4:27 PM, Russell King
 rmk+ker...@arm.linux.org.uk wrote:
  Subsystems such as ALSA, DRM and others require a single card-level
  device structure to represent a subsystem.  However, firmware tends to
  describe the individual devices and the connections between them.
 
  Therefore, we need a way to gather up the individual component devices
  together, and indicate when we have all the component devices.
 
  We do this in DT by providing a superdevice node which specifies
  the components, eg:
 
  imx-drm {
  compatible = fsl,drm;
  crtcs = ipu1;
  connectors = hdmi;
  };
 

 Hi Russell,
 This looks really good. I'd definitely like to use it for the exynos
 drm driver both to bind the various IP blocks together, and also to
 pull in any attached bridges that are specified in the device tree.
 Along those lines, it might be worthwhile to pull some of the master
 bind functionality in your next patch into drm helpers so other
 drivers can use them, and so we have concrete bindings across drm.

 Which bits do you think would be useful to move into the core?
 imx_drm_add_components() is rather imx-drm specific, especially for
 the CRTCs - it carries the knowledge that the OF device to be matched
 can be found in the _parent_ device, rather than the device registered
 into the component helpers.


Yeah, I was thinking of imx_drm_add_components() actually. It probably
doesn't make sense to enforce the parent relationship in a generic
manner, but it would be nice to have a helper which added the various
drm components (crtc/encoder/bridge/connector) with a consistent
binding.

We have 3 different exynos boards which would have the following
superdevices (please forgive my hypothetical syntax/naming):

Board 1:
exynos-drm {
compatible = exynos,drm;
crtcs = fimd1, mixer1;
encoders = dp1, hdmi1;
bridges = ptn3460 dp1;
connectors = ptn3460, hdmi1;
};

Board 2:
exynos-drm {
compatible = exynos,drm;
crtcs = fimd1, mixer1;
encoders = dp1, hdmi1;
bridges = ps8622 dp1, anx7808 hdmi1;
connectors = ps8622, anx7808;
};

Board 3:
exynos-drm {
compatible = exynos,drm;
crtcs = fimd1, mixer1;
encoders = dp1, hdmi1;
connectors = dp1, hdmi1;
};

In addition to enforcing common bindings across drivers, we could also
assign bridges to encoders from the dt. The bridge-encoder
relationship is 1:1 at the moment, and the bridge driver can be a
completely separate entity from the encoder. Allowing assignment from
the dt means the encoder never needs to know whether a bridge is
connected.

Right now the encoder must enumerate all possible bridges to see if
they are present (check out [PATCH v3 31/32] drm/exynos: Move lvds
bridge discovery into DP driver for an example of what I mean).

Sean

 --
 FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
 in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
 Estimate before purchase was up to 13.2Mbit.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems

2014-01-07 Thread Sean Paul 
On Thu, Jan 2, 2014 at 4:27 PM, Russell King
rmk+ker...@arm.linux.org.uk wrote:
 Subsystems such as ALSA, DRM and others require a single card-level
 device structure to represent a subsystem.  However, firmware tends to
 describe the individual devices and the connections between them.

 Therefore, we need a way to gather up the individual component devices
 together, and indicate when we have all the component devices.

 We do this in DT by providing a superdevice node which specifies
 the components, eg:

 imx-drm {
 compatible = fsl,drm;
 crtcs = ipu1;
 connectors = hdmi;
 };


Hi Russell,
This looks really good. I'd definitely like to use it for the exynos
drm driver both to bind the various IP blocks together, and also to
pull in any attached bridges that are specified in the device tree.
Along those lines, it might be worthwhile to pull some of the master
bind functionality in your next patch into drm helpers so other
drivers can use them, and so we have concrete bindings across drm.
Make sense?

Sean



 The superdevice is declared into the component support, along with the
 subcomponents.  The superdevice receives callbacks to locate the
 subcomponents, and identify when all components are present.  At this
 point, we bind the superdevice, which causes the appropriate subsystem
 to be initialised in the conventional way.

 When any of the components or superdevice are removed from the system,
 we unbind the superdevice, thereby taking the subsystem down.

 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 ---
  drivers/base/Makefile |2 +-
  drivers/base/component.c  |  379 
 +
  include/linux/component.h |   31 
  3 files changed, 411 insertions(+), 1 deletions(-)
  create mode 100644 drivers/base/component.c
  create mode 100644 include/linux/component.h

 diff --git a/drivers/base/Makefile b/drivers/base/Makefile
 index 94e8a80e87f8..870ecfd503af 100644
 --- a/drivers/base/Makefile
 +++ b/drivers/base/Makefile
 @@ -1,6 +1,6 @@
  # Makefile for the Linux device tree

 -obj-y  := core.o bus.o dd.o syscore.o \
 +obj-y  := component.o core.o bus.o dd.o syscore.o \
driver.o class.o platform.o \
cpu.o firmware.o init.o map.o devres.o \
attribute_container.o transport_class.o \
 diff --git a/drivers/base/component.c b/drivers/base/component.c
 new file mode 100644
 index ..5492cd8d2247
 --- /dev/null
 +++ b/drivers/base/component.c
 @@ -0,0 +1,379 @@
 +/*
 + * Componentized device handling.
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as
 + * published by the Free Software Foundation.
 + *
 + * This is work in progress.  We gather up the component devices into a list,
 + * and bind them when instructed.  At the moment, we're specific to the DRM
 + * subsystem, and only handles one master device, but this doesn't have to be
 + * the case.
 + */
 +#include linux/component.h
 +#include linux/device.h
 +#include linux/kref.h
 +#include linux/list.h
 +#include linux/module.h
 +#include linux/mutex.h
 +#include linux/slab.h
 +
 +struct master {
 +   struct list_head node;
 +   struct list_head components;
 +   bool bound;
 +
 +   const struct component_master_ops *ops;
 +   struct device *dev;
 +};
 +
 +struct component {
 +   struct list_head node;
 +   struct list_head master_node;
 +   struct master *master;
 +   bool bound;
 +
 +   const struct component_ops *ops;
 +   struct device *dev;
 +};
 +
 +static DEFINE_MUTEX(component_mutex);
 +static LIST_HEAD(component_list);
 +static LIST_HEAD(masters);
 +
 +static struct master *__master_find(struct device *dev, const struct 
 component_master_ops *ops)
 +{
 +   struct master *m;
 +
 +   list_for_each_entry(m, masters, node)
 +   if (m-dev == dev  (!ops || m-ops == ops))
 +   return m;
 +
 +   return NULL;
 +}
 +
 +/* Attach an unattached component to a master. */
 +static void component_attach_master(struct master *master, struct component 
 *c)
 +{
 +   c-master = master;
 +
 +   list_add_tail(c-master_node, master-components);
 +}
 +
 +/* Detach a component from a master. */
 +static void component_detach_master(struct master *master, struct component 
 *c)
 +{
 +   list_del(c-master_node);
 +
 +   c-master = NULL;
 +}
 +
 +int component_master_add_child(struct master *master,
 +   int (*compare)(struct device *, void *), void *compare_data)
 +{
 +   struct component *c;
 +   int ret = -ENXIO;
 +
 +   list_for_each_entry(c, component_list, node) {
 +   if (c-master)
 +   continue;
 +
 +   if (compare(c-dev, compare_data)) {
 +