[PATCH 2/2] drm/msm/hdmi: add hdmi hdcp support
Hi Bjorn,
> On Tue, Dec 2, 2014 at 8:46 PM, Bjorn Andersson wrote:
>> On Mon, Dec 1, 2014 at 1:56 PM, Jilai Wang
>> wrote:
>>> Add HDMI HDCP support including HDCP PartI/II/III authentication.
>>>
>>> Signed-off-by: Jilai Wang
>>> ---
>>
>> Hi Jilai,
>>
>> [..]
>>
>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>
>> [..]
>>
>>>
>>> @@ -119,6 +137,22 @@ struct hdmi *hdmi_init(struct drm_device *dev,
>>> struct drm_encoder *encoder)
>>> goto fail;
>>> }
>>>
>>> + /* HDCP needs physical address of hdmi register */
>>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>> + config->mmio_name);
>>
>> Is this guaranteed to be available at all times? You should probably
>> do some error handling here.
>
> Hi Bjorn,
>
> (as mentioned on irc, but just repeating here for posterity)
>
> this is actually ok in this case, since the previous ioremap would have
> failed.
>
>>> + hdmi->mmio_phy_addr = res->start;
>>> +
>>> + if (config->qfprom_mmio_name) {
>>
>> Should this check really be here? This will always be set if CONFIG_OF
>> is set
>> and never otherwise and that seems strange to me.
>>
>> Perhaps you should add the string to the !CONFIG_OF platforms as well or
>> simply
>> add #ifdef CONFIG_OF around this section if that's what you really want.
>> (but
>> seems more like you forgot the non-of case).
>
> or just not cared enough to make HDCP (since it is optional) work on
> old pre-DT vendor kernel ;-)
>
> fwiw, eventually the !CONFIG_OF stuff will go away.. it just exists
> because some devices and some use-cases still need old downstream
> kernels :-(
>
>>> + hdmi->qfprom_mmio = msm_ioremap(pdev,
>>> + config->qfprom_mmio_name, "HDMI_QFPROM");
>>
>>
>> Is this a special hdmi qfprom or are you ioremapping _the_ qfprom here?
>>
>> If so I did suggest that we expose it as a syscon but I think Stephen
>> Boyd had
>> some other ideas.
>
> afaict (but Jilai can correct me), it is *the* qfprom.. that seems to
> be how things worked in android kernels. Some better mechanism would
> be really nice.
>
>>> + if (IS_ERR(hdmi->qfprom_mmio)) {
>>> + dev_info(&pdev->dev, "can't find qfprom
>>> resource\n");
>>> + hdmi->qfprom_mmio = NULL;
>>> + }
>>> + } else {
>>> + hdmi->qfprom_mmio = NULL;
>>
>> hdmi_qfprom_read() seems to be called and read from qfprom_mmio no
>> matter how
>> this ended. Are you sure this (both error paths) shouldn't be handled as
>> a
>> fatal error?
>
> hdmi_hdcp_init() fails (and then we continue without HDCP) if
> qfprom_mmio is NULL..
>
>> 'hdmi' is kzalloc and hence already NULL.
>>
>> [..]
>>
>>> @@ -205,6 +241,13 @@ struct hdmi *hdmi_init(struct drm_device *dev,
>>> struct drm_encoder *encoder)
>>> goto fail;
>>> }
>>>
>>> + hdmi->hdcp_ctrl = hdmi_hdcp_init(hdmi);
>>> + if (IS_ERR(hdmi->hdcp_ctrl)) {
>>> + ret = PTR_ERR(hdmi->hdcp_ctrl);
>>> + dev_warn(dev->dev, "failed to init hdcp:
>>> %d(disabled)\n", ret);
>>> + hdmi->hdcp_ctrl = NULL;
>>
>> So either you treat this as an error or you don't.
>>
>> If you're fine continuing execution without hdcp_ctrl then you shouldn't
>> set
>> ret. But in that case it you should probably not print a warning every
>> time you
>> enter hdmi_hdcp_on() and an error on hdmi_hdcp_off().
>
> agreed, I think it would be better for hdcp_on/off() to take struct
> hdmi_hdcp_ctrl as param (and just not be called if hdmi->hdcp_ctrl is
> null)
>
> [snip]
>
>> [..]
>>
>>> +
>>> +struct hdmi_hdcp_reg_data {
>>> + uint32_t reg_id;
>>
>> You should use u32 instead of uint32_t in the kernel.
>
> tbh, I'd prefer sticking to stdint types.. before stdint was a thing,
> u32 made sense
>
>>> + uint32_t off;
>>> + char *name;
>>> + uint32_t reg_val;
>>> +};
>>> +
>>> +struct hdmi_hdcp_ctrl {
>>> + struct hdmi *hdmi;
>>> + uint32_t auth_retries;
>>> + uint32_t tz_hdcp;
>>
>> Turn this into a bool named something like has_tz_hdcp instead, as
>> that's what
>> it really means.
>>
>>> + enum hdmi_hdcp_state hdcp_state;
>>> + struct mutex state_mutex;
>>> + struct delayed_work hdcp_reauth_work;
>>> + struct delayed_work hdcp_auth_part1_1_work;
>>> + struct delayed_work hdcp_auth_part1_2_work;
>>> + struct work_struct hdcp_auth_part1_3_work;
>>> + struct delayed_work hdcp_auth_part2_1_work;
>>> + struct delayed_work hdcp_auth_part2_2_work;
>>> + struct delayed_work hdcp_auth_part2_3_work;
>>> + struct delayed_work hdcp_auth_part2_4_work;
>>> + struct work_struct hdcp_auth_prepare_work;
>>
>> You shouldn't use "work" as a way to express states in your state
>> machine.
>> Better have 1 auth work function that does all these steps, probably
>> having
>> them split i
[PATCH 2/2] drm/msm/hdmi: add hdmi hdcp support
> On Wed, Dec 3, 2014 at 9:16 AM, wrote: > [..] > + enum hdmi_hdcp_state hdcp_state; > + struct mutex state_mutex; > + struct delayed_work hdcp_reauth_work; > + struct delayed_work hdcp_auth_part1_1_work; > + struct delayed_work hdcp_auth_part1_2_work; > + struct work_struct hdcp_auth_part1_3_work; > + struct delayed_work hdcp_auth_part2_1_work; > + struct delayed_work hdcp_auth_part2_2_work; > + struct delayed_work hdcp_auth_part2_3_work; > + struct delayed_work hdcp_auth_part2_4_work; > + struct work_struct hdcp_auth_prepare_work; You shouldn't use "work" as a way to express states in your state machine. Better have 1 auth work function that does all these steps, probably having them split in functions just like you do now. That way you can have 1 function running the pass of authentication, starting by checking if you're reauthing or not then processing each step one by one, sleeping inbetween them. You can have the functions return -EAGAIN to indicate that you need to retry the current operation and so on. This would split the state machine from the state executioners and simplify your code. >>> >>> As a side note (disclaimer, I'm not an hdcp expert), but I wonder if >>> eventually some of that should be extracted out into some helpers in >>> drm core. I guess that is something we'll figure out when a 2nd >>> driver gains upstream HDCP support. One big work w/ msleep()'s does >>> sound like it would be easier to understand (and therefore easier to >>> refactor out into helpers). >>> >>> [snip] >>> >> >> The reason that I break the partI/PartII work into these small works >> because I want to avoid to use msleep in work. >> Otherwise cancel a HDCP work may cause long delay up to several seconds. >> > > I definitely think the steps are nice size and make things easy to > understand. > > If you make the steps that require a retry return out to the main > state handling function with a -EAGAIN, then you can have check if you > should retry or abort based on cancellation. Giving you the worst case > cancellation time of the longest sleep... > > As Rob suggest you could use wait_event_timeout() or something to > improve this, but on the other hand the worst case here seems to be > the HZ/2 after prepare_auth; others are HZ/8, HZ/20 and HZ/50; not > "seconds". > > So I would start with a simple msleep() for implementation simplicity > and then enhance that in a follow up commit (if needed)... > > Regards, > Bjorn > The worst wait time could be 5 seconds if the downstream device is a REPEATER. The maximum retry time is set to 100 and the delay for each time is HZ/20, then the maximum wait time before abort will be 5 seconds. As you suggested, I can use the flag to notify the retry process to abort earlier in case this work needs to be canceled which can reduce the delay to HZ/20 then. Or as Rob's suggestion to wait for hpd event. Thanks, Jilai
[PATCH] drm/msm: Refactor msm drm driver by introducing msm_drm_sub_dev
> On Thu, Mar 12, 2015 at 4:48 PM, Jilai Wang wrote:
>> Introduce msm_drm_sub_dev for each mdp interface component such as
>> HDMI/eDP/DSI to contain common information shared with MDP.
>>
>> Signed-off-by: Jilai Wang
>> ---
>> drivers/gpu/drm/msm/edp/edp.c | 18 +--
>> drivers/gpu/drm/msm/edp/edp.h | 1 +
>> drivers/gpu/drm/msm/hdmi/hdmi.c | 22 ++---
>> drivers/gpu/drm/msm/hdmi/hdmi.h | 1 +
>> drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 3 +-
>> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 56
>> -
>> drivers/gpu/drm/msm/msm_drv.h | 23 +-
>> 7 files changed, 80 insertions(+), 44 deletions(-)
>
> So a couple comments..
>
> 1) I kinda prefer having some to_hdmi/to_edp/etc macros rather than
> just open coding container_of().. I guess kind of a minor thing, but
> it keeps things consistent with how "inheritance" is handled elsewhere
> (like to_mdp5_crtc(), etc)
>
> 2) I'd be a bit more enthused by this when it actually results in a
> negative diffstat, rather than a positive one. Not saying it isn't
> something we should do at some point, but at this point I'm leaning
> towards rebasing the DSI patcheset to not depend on this for now.
Actually most of the negative diffstat is in mdp5_kms.c which moves the
modeset_init out of construct encoder. And it is required by DSI change.
>
> BR,
> -R
>
>>
>> diff --git a/drivers/gpu/drm/msm/edp/edp.c
>> b/drivers/gpu/drm/msm/edp/edp.c
>> index 0940e84..8d8b7e9 100644
>> --- a/drivers/gpu/drm/msm/edp/edp.c
>> +++ b/drivers/gpu/drm/msm/edp/edp.c
>> @@ -14,6 +14,9 @@
>> #include
>> #include "edp.h"
>>
>> +static int msm_edp_modeset_init(struct msm_drm_sub_dev *base,
>> + struct drm_device *dev);
>> +
>> static irqreturn_t edp_irq(int irq, void *dev_id)
>> {
>> struct msm_edp *edp = dev_id;
>> @@ -63,6 +66,8 @@ static struct msm_edp *edp_init(struct platform_device
>> *pdev)
>> if (ret)
>> goto fail;
>>
>> + edp->base.modeset_init = msm_edp_modeset_init;
>> +
>> return edp;
>>
>> fail:
>> @@ -82,7 +87,8 @@ static int edp_bind(struct device *dev, struct device
>> *master, void *data)
>> edp = edp_init(to_platform_device(dev));
>> if (IS_ERR(edp))
>> return PTR_ERR(edp);
>> - priv->edp = edp;
>> +
>> + priv->edp = &edp->base;
>>
>> return 0;
>> }
>> @@ -144,13 +150,19 @@ void __exit msm_edp_unregister(void)
>> }
>>
>> /* Second part of initialization, the drm/kms level modeset_init */
>> -int msm_edp_modeset_init(struct msm_edp *edp, struct drm_device *dev,
>> - struct drm_encoder *encoder)
>> +static int msm_edp_modeset_init(struct msm_drm_sub_dev *base,
>> + struct drm_device *dev)
>> {
>> + struct msm_edp *edp = container_of(base, struct msm_edp, base);
>> struct platform_device *pdev = edp->pdev;
>> struct msm_drm_private *priv = dev->dev_private;
>> + struct drm_encoder *encoder;
>> int ret;
>>
>> + if (WARN_ON(base->num_encoders != 1))
>> + return -EINVAL;
>> +
>> + encoder = base->encoders[0];
>> edp->encoder = encoder;
>> edp->dev = dev;
>>
>> diff --git a/drivers/gpu/drm/msm/edp/edp.h
>> b/drivers/gpu/drm/msm/edp/edp.h
>> index ba5bedd..00eff68 100644
>> --- a/drivers/gpu/drm/msm/edp/edp.h
>> +++ b/drivers/gpu/drm/msm/edp/edp.h
>> @@ -31,6 +31,7 @@ struct edp_aux;
>> struct edp_phy;
>>
>> struct msm_edp {
>> + struct msm_drm_sub_dev base;
>> struct drm_device *dev;
>> struct platform_device *pdev;
>>
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c
>> b/drivers/gpu/drm/msm/hdmi/hdmi.c
>> index 9a8a825..9e886ec 100644
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
>> @@ -19,6 +19,9 @@
>> #include
>> #include "hdmi.h"
>>
>> +static int hdmi_modeset_init(struct msm_drm_sub_dev *base,
>> + struct drm_device *dev);
>> +
>> void hdmi_set_mode(struct hdmi *hdmi, bool power_on)
>> {
>> uint32_t ctrl = 0;
>> @@ -197,6 +200,8 @@ static struct hdmi *hdmi_init(struct platform_device
>> *pdev)
>> goto fail;
>> }
>>
>> + hdmi->base.modeset_init = hdmi_modeset_init;
>> +
>> return hdmi;
>>
>> fail:
>> @@ -214,13 +219,19 @@ fail:
>> * should be handled in hdmi_init() so that failure happens from
>> * hdmi sub-device's probe.
>> */
>> -int hdmi_modeset_init(struct hdmi *hdmi,
>> - struct drm_device *dev, struct drm_encoder *encoder)
>> +static int hdmi_modeset_init(struct msm_drm_sub_dev *base,
>> + struct drm_device *dev)
>> {
>> + struct hdmi *hdmi = container_of(base, struct hdmi, base);
>> struct msm_drm_private *priv = dev->dev_private;
>> struct platform_device *pdev = hdmi->pdev;
>> + struct drm_encoder *encoder;
>> int ret;
>>
>> + if (WARN_ON(
[PATCH] drm/msm: Refactor msm drm driver by introducing msm_drm_sub_dev
> On Tue, Mar 24, 2015 at 11:18 AM, wrote:
>>
>>> On Thu, Mar 12, 2015 at 4:48 PM, Jilai Wang
>>> wrote:
Introduce msm_drm_sub_dev for each mdp interface component such as
HDMI/eDP/DSI to contain common information shared with MDP.
Signed-off-by: Jilai Wang
---
drivers/gpu/drm/msm/edp/edp.c | 18 +--
drivers/gpu/drm/msm/edp/edp.h | 1 +
drivers/gpu/drm/msm/hdmi/hdmi.c | 22 ++---
drivers/gpu/drm/msm/hdmi/hdmi.h | 1 +
drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 3 +-
drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 56
-
drivers/gpu/drm/msm/msm_drv.h | 23 +-
7 files changed, 80 insertions(+), 44 deletions(-)
>>>
>>> So a couple comments..
>>>
>>> 1) I kinda prefer having some to_hdmi/to_edp/etc macros rather than
>>> just open coding container_of().. I guess kind of a minor thing, but
>>> it keeps things consistent with how "inheritance" is handled elsewhere
>>> (like to_mdp5_crtc(), etc)
>>>
>>> 2) I'd be a bit more enthused by this when it actually results in a
>>> negative diffstat, rather than a positive one. Not saying it isn't
>>> something we should do at some point, but at this point I'm leaning
>>> towards rebasing the DSI patcheset to not depend on this for now.
>> Actually most of the negative diffstat is in mdp5_kms.c which moves the
>> modeset_init out of construct encoder. And it is required by DSI change.
>
>
> What I meant is that it adds more lines of code than it removes.. for
> refactoring/cleanup type things, I'd generally prefer things that work
> out the other way around (removing more lines than they add)..
> Anyways, if I drop this patch, I'll rebase the DSI patches.. that is
> fine.
>
> By the time I get DSI also working on mdp4, it might get to the point
> where this sort of change actually removes more lines than it adds, so
> that might be the time to revisit. We may also be able to simplify it
> a bit.. I'm still thinking about it, I haven't completely decided
> yet.
ok.
>
> BR,
> -R
>
>
>>>
>>> BR,
>>> -R
>>>
diff --git a/drivers/gpu/drm/msm/edp/edp.c
b/drivers/gpu/drm/msm/edp/edp.c
index 0940e84..8d8b7e9 100644
--- a/drivers/gpu/drm/msm/edp/edp.c
+++ b/drivers/gpu/drm/msm/edp/edp.c
@@ -14,6 +14,9 @@
#include
#include "edp.h"
+static int msm_edp_modeset_init(struct msm_drm_sub_dev *base,
+ struct drm_device *dev);
+
static irqreturn_t edp_irq(int irq, void *dev_id)
{
struct msm_edp *edp = dev_id;
@@ -63,6 +66,8 @@ static struct msm_edp *edp_init(struct
platform_device
*pdev)
if (ret)
goto fail;
+ edp->base.modeset_init = msm_edp_modeset_init;
+
return edp;
fail:
@@ -82,7 +87,8 @@ static int edp_bind(struct device *dev, struct
device
*master, void *data)
edp = edp_init(to_platform_device(dev));
if (IS_ERR(edp))
return PTR_ERR(edp);
- priv->edp = edp;
+
+ priv->edp = &edp->base;
return 0;
}
@@ -144,13 +150,19 @@ void __exit msm_edp_unregister(void)
}
/* Second part of initialization, the drm/kms level modeset_init */
-int msm_edp_modeset_init(struct msm_edp *edp, struct drm_device *dev,
- struct drm_encoder *encoder)
+static int msm_edp_modeset_init(struct msm_drm_sub_dev *base,
+ struct drm_device *dev)
{
+ struct msm_edp *edp = container_of(base, struct msm_edp,
base);
struct platform_device *pdev = edp->pdev;
struct msm_drm_private *priv = dev->dev_private;
+ struct drm_encoder *encoder;
int ret;
+ if (WARN_ON(base->num_encoders != 1))
+ return -EINVAL;
+
+ encoder = base->encoders[0];
edp->encoder = encoder;
edp->dev = dev;
diff --git a/drivers/gpu/drm/msm/edp/edp.h
b/drivers/gpu/drm/msm/edp/edp.h
index ba5bedd..00eff68 100644
--- a/drivers/gpu/drm/msm/edp/edp.h
+++ b/drivers/gpu/drm/msm/edp/edp.h
@@ -31,6 +31,7 @@ struct edp_aux;
struct edp_phy;
struct msm_edp {
+ struct msm_drm_sub_dev base;
struct drm_device *dev;
struct platform_device *pdev;
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c
b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 9a8a825..9e886ec 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -19,6 +19,9 @@
#include
#include "hdmi.h"
+static int hdmi_modeset_init(struct msm_drm_sub_dev *base,
+ struct drm_device *dev);
+
void hdmi_set_mode(struct hdmi *hdmi, bool p
[PATCH 1/2] drm/doc: Add hflip/vflip property descriptions in msm
Thanks for this information, Daniel.
I will change it to use rotation prop in next patch.
> On Mon, Jul 27, 2015 at 06:57:33PM -0400, Jilai Wang wrote:
>> Add plane properties hflip/vflip which are used in MDP driver to flip
>> the input horizontally/vertically.
>>
>> Signed-off-by: Jilai Wang
>
> Is the existing rotation prop with flip masks not enough?
> -Daniel
>
>> ---
>> Documentation/DocBook/drm.tmpl | 18 --
>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/DocBook/drm.tmpl
>> b/Documentation/DocBook/drm.tmpl
>> index d9f5613..bef6d34 100644
>> --- a/Documentation/DocBook/drm.tmpl
>> +++ b/Documentation/DocBook/drm.tmpl
>> @@ -3501,8 +3501,8 @@ void intel_crt_init(struct drm_device *dev)
>> TBD
>>
>>
>> -msm
>> -Generic
>> +msm
>> +Generic
>> "premultiplied"
>> ENUM
>> { "false", "true" }
>> @@ -3523,6 +3523,20 @@ void intel_crt_init(struct drm_device *dev)
>> Plane
>> property to set plane's z position during
>> blending
>>
>> +
>> +"hflip"
>> +ENUM
>> +{ "off", "on" }
>> +Plane
>> +property to flip the input horizontally
>> +
>> +
>> +"vflip"
>> +ENUM
>> +{ "off", "on" }
>> +Plane
>> +property to flip the input vertically
>> +
>>
>>
>>
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project
>>
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>
[PATCH] drm/msm/mdp5:Add DMA pipe planes for MDP5
Hi Rob,
DMA pipes can be configured to work in either line mode or block mode.
In line mode, it is the same as RGB/VIG pipes except no CSC/SCALE/PP/...
support. So it can be used by any CRTC.
In block mode, it is used as a rotator with writeback(0/1) interface which
is not covered by this change.
> On Tue, Jul 7, 2015 at 5:17 PM, Jilai Wang wrote:
>> This change is to add planes which use DMA pipes for MDP5.
>
> are DMA pipes only supporting memory->memory operation, or am I
> reading too much into the name "DMA"? I'm wondering if we need to fix
> the possible_crtcs param that mdp5_plane_init passes to
> drm_universal_plane_init()?
>
> BR,
> -R
>
>
>> Signed-off-by: Jilai Wang
>> ---
>> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c | 23 ---
>> 1 file changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>> index cbda41d..f40896d 100644
>> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
>> @@ -316,9 +316,12 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
>> static const enum mdp5_pipe crtcs[] = {
>> SSPP_RGB0, SSPP_RGB1, SSPP_RGB2, SSPP_RGB3,
>> };
>> - static const enum mdp5_pipe pub_planes[] = {
>> + static const enum mdp5_pipe vig_planes[] = {
>> SSPP_VIG0, SSPP_VIG1, SSPP_VIG2, SSPP_VIG3,
>> };
>> + static const enum mdp5_pipe dma_planes[] = {
>> + SSPP_DMA0, SSPP_DMA1,
>> + };
>> struct drm_device *dev = mdp5_kms->dev;
>> struct msm_drm_private *priv = dev->dev_private;
>> const struct mdp5_cfg_hw *hw_cfg;
>> @@ -361,12 +364,26 @@ static int modeset_init(struct mdp5_kms *mdp5_kms)
>> for (i = 0; i < hw_cfg->pipe_vig.count; i++) {
>> struct drm_plane *plane;
>>
>> - plane = mdp5_plane_init(dev, pub_planes[i], false,
>> + plane = mdp5_plane_init(dev, vig_planes[i], false,
>> hw_cfg->pipe_vig.base[i],
>> hw_cfg->pipe_vig.caps);
>> if (IS_ERR(plane)) {
>> ret = PTR_ERR(plane);
>> dev_err(dev->dev, "failed to construct %s plane:
>> %d\n",
>> - pipe2name(pub_planes[i]), ret);
>> + pipe2name(vig_planes[i]), ret);
>> + goto fail;
>> + }
>> + }
>> +
>> + /* DMA planes */
>> + for (i = 0; i < hw_cfg->pipe_dma.count; i++) {
>> + struct drm_plane *plane;
>> +
>> + plane = mdp5_plane_init(dev, dma_planes[i], false,
>> + hw_cfg->pipe_dma.base[i],
>> hw_cfg->pipe_dma.caps);
>> + if (IS_ERR(plane)) {
>> + ret = PTR_ERR(plane);
>> + dev_err(dev->dev, "failed to construct %s plane:
>> %d\n",
>> + pipe2name(dma_planes[i]), ret);
>> goto fail;
>> }
>> }
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project
>>
>
[PATCH 2/2] drm/msm/hdmi: add hdmi hdcp support (V2)
There is one issue to use i2c_smbus_XX functions: i2c_smbus_read_i2c_block_data has limitation with the maximum count I2C_SMBUS_BLOCK_MAX. But in function hdmi_hdcp_recv_ksv_fifo, since the downstream ksv_fifo size will exceed this limitation and must be read in a single transaction, we can't use this function then. > On Fri, Jan 30, 2015 at 2:39 PM, Bjorn Andersson wrote: >> On Fri, Jan 30, 2015 at 1:51 PM, Bjorn Andersson wrote: >>> On Tue, Jan 13, 2015 at 12:43 PM, Jilai Wang >>> wrote: Add HDMI HDCP support including HDCP PartI/II/III authentication. V1: Initial Change V2: Address Bjorn&Rob's comments Refactor the authentication process to use single work instead of multiple work for different authentication stages. >>> Looks cleaner and the SCM parts look good now. >>> >>> But the ddc communication still makes me wonder, see below. >>> >> >> Rob made me aware about the fact that the hdmi driver is both >> implementing a driver for the i2c controller and now for the hdcp >> client and hence doesn't have an i2c_client handle. >> >> So unless this is redesigned to be split in a separate i2c client >> driver we probably have to have the ddc functions like this. >> >> I'm fine with this as is for now. >> > > After digging some more in the i2c stack I found others using > i2c_new_dummy() to create a "dummy" i2c client from and adaptor and an > address. > By introducing that we could make hdmi->i2c an actual i2c_client and > use the client api in here. > > Regards, > Bjorn >
[PATCH 2/3] drm:msm: Initial Add Writeback Support
Thanks Paul. Some comments embedded and for the rest I will update the
code accordingly.
> A few nits follow.
>
> On Wed, 2015-04-01 at 17:12 -0400, Jilai Wang wrote:
>> --- a/drivers/gpu/drm/msm/Kconfig
>> +++ b/drivers/gpu/drm/msm/Kconfig
>
>> +config DRM_MSM_WB
>> +bool "Enable writeback support for MSM modesetting driver"
>> +depends on DRM_MSM
>> +depends on VIDEO_V4L2
>> +select VIDEOBUF2_CORE
>> +default y
>> +help
>> + Choose this option if you have a need to support writeback
>> + connector.
>
> DRM_MSM_WB is a bool symbol...
>
>> --- a/drivers/gpu/drm/msm/Makefile
>> +++ b/drivers/gpu/drm/msm/Makefile
>
>> +msm-$(CONFIG_DRM_MSM_WB) += \
>> +mdp/mdp5/mdp5_wb_encoder.o \
>> +mdp/mdp_wb/mdp_wb.o \
>> +mdp/mdp_wb/mdp_wb_connector.o \
>> +mdp/mdp_wb/mdp_wb_v4l2.o
>
> so mdp_wb_v4l2.o will never be part of a module.
If CONFIG-DRM_MSM_WB is y, then all wb related files (including
mdp_wb_v4l2.o) will be added to msm-y, then be linked to msm.o
obj-$(CONFIG_DRM_MSM) += msm.o
Refer to document http://lwn.net/Articles/21835/ (section 3.3),
it should be able to be built-in to kernel or as a module.
>
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb_v4l2.c
>
>> +#include
>
> This include is needed mostly for module_param(), right?
>
>> +#define MSM_WB_MODULE_NAME "msm_wb"
>
> MSM_WB_DRIVER_NAME? But see below.
>
>> +static unsigned debug;
>> +module_param(debug, uint, 0644);
>
> debug is basically a boolean type of flag. Would
> static bool debug;
> module_param(debug, bool, 0644);
>
> work?
>
>> +MODULE_PARM_DESC(debug, "activates debug info");
>
> No one is ever going to see this description.
>
>> +#define dprintk(dev, level, fmt, arg...) \
>> +v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
>
> All instances of dprintk() that I found had level set to 1, so the above
> could be simplified a bit:
> #define dprintk(dev, fmt, arg...) \
> v4l2_dbg(1, debug, &dev->v4l2_dev, fmt, ## arg)
>
> But perhaps pr_debug() and the dynamic debug facility could be used
> instead of module_param(), dprintk() and v4l2_dbg(). Not sure which
> approach is easier.
>
>> +static const struct v4l2_file_operations msm_wb_v4l2_fops = {
>> +.owner = THIS_MODULE,
>
> THIS_MODULE will basically be equivalent to NULL.
>
>> +.open = v4l2_fh_open,
>> +.release = vb2_fop_release,
>> +.poll = vb2_fop_poll,
>> +.unlocked_ioctl = video_ioctl2,
>> +};
>
>> +int msm_wb_v4l2_init(struct msm_wb *wb)
>> +{
>> +struct msm_wb_v4l2_dev *dev;
>> +struct video_device *vfd;
>> +struct vb2_queue *q;
>> +int ret;
>> +
>> +dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> +if (!dev)
>> +return -ENOMEM;
>> +
>> +snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name),
>> +"%s", MSM_WB_MODULE_NAME);
>
> It looks like you can actually drop the #define and merge the last two
> arguments to snprintf() into just "msm_wb".
>
>> +ret = v4l2_device_register(NULL, &dev->v4l2_dev);
>> +if (ret)
>> +goto free_dev;
>> +
>> +/* default ARGB 640x480 */
>> +dev->fmt = get_format(V4L2_PIX_FMT_RGB32);
>> +dev->width = 640;
>> +dev->height = 480;
>> +
>> +/* initialize queue */
>> +q = &dev->vb_vidq;
>> +q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
>> +q->io_modes = VB2_DMABUF;
>> +q->drv_priv = dev;
>> +q->buf_struct_size = sizeof(struct msm_wb_v4l2_buffer);
>> +q->ops = &msm_wb_vb2_ops;
>> +q->mem_ops = &msm_wb_vb2_mem_ops;
>> +q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> +
>> +ret = vb2_queue_init(q);
>> +if (ret)
>> +goto unreg_dev;
>> +
>> +mutex_init(&dev->mutex);
>> +
>> +vfd = &dev->vdev;
>> +*vfd = msm_wb_v4l2_template;
>> +vfd->debug = debug;
>
> I couldn't find a member of struct video_device named debug. Where does
> that come from? Because, as far as I can see, this won't compile.
yes, it's there:
http://lxr.free-electrons.com/source/include/media/v4l2-dev.h#L127
>
>> +vfd->v4l2_dev = &dev->v4l2_dev;
>> +vfd->queue = q;
>> +
>> +/*
>> + * Provide a mutex to v4l2 core. It will be used to protect
>> + * all fops and v4l2 ioctls.
>> + */
>> +vfd->lock = &dev->mutex;
>> +video_set_drvdata(vfd, dev);
>> +
>> +ret = video_register_device(vfd, VFL_TYPE_GRABBER, -1);
>> +if (ret < 0)
>> +goto unreg_dev;
>> +
>> +dev->wb = wb;
>> +wb->wb_v4l2 = dev;
>> +v4l2_info(&dev->v4l2_dev, "V4L2 device registered as %s\n",
>> + video_device_node_name(vfd));
>> +
>> +return 0;
>> +
>> +unreg_dev:
>> +v4l2_device_unregister(&dev->v4l2_dev);
>> +free_dev:
>> +kfree(dev);
>> +return ret;
>> +}
>
>
> Paul Bolle
>
>
[PATCH 2/3] drm:msm: Initial Add Writeback Support
Thanks Emil. Please check the comments embedded and for the rest I will
update the code.
> Hi Jilai,
>
> Just a few questions, not really a review as I'm not that familiar
> with the code.
>
>
>> +++ b/drivers/gpu/drm/msm/Kconfig
>> @@ -27,6 +27,16 @@ config DRM_MSM_FBDEV
>> support. Note that this support also provide the linux console
>> support on top of the MSM modesetting driver.
>>
>> +config DRM_MSM_WB
>> + bool "Enable writeback support for MSM modesetting driver"
>> + depends on DRM_MSM
>> + depends on VIDEO_V4L2
>> + select VIDEOBUF2_CORE
>> + default y
>> + help
>> + Choose this option if you have a need to support writeback
>> + connector.
>> +
> Is it worth mentioning which devices/SoCs have such connector ?
If the devices have WB connector, it will be added in the device tree.
>
>
>> +++ b/drivers/gpu/drm/msm/Makefile
>> @@ -1,4 +1,5 @@
>> -ccflags-y := -Iinclude/drm -Idrivers/gpu/drm/msm
>> +ccflags-y := -Iinclude/drm -Idrivers/gpu/drm/msm
>> -Idrivers/gpu/drm/msm/mdp_wb
>> +ccflags-$(CONFIG_DRM_MSM_WB) += -Idrivers/gpu/drm/msm/mdp/mdp_wb
>>
> I think you only want the second line here.
>
>
>> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_wb_encoder.c
>
>> +static struct msm_bus_paths mdp_bus_usecases[] = { {
>> + .num_paths = 1,
>> + .vectors = &mdp_bus_vectors[0],
>> +}, {
>> + .num_paths = 1,
>> + .vectors = &mdp_bus_vectors[1],
>> +} };
> The following formatting seems more common:
>
> static struct foo foo1[] = {
> {
> bar
> }
> };
>
>
>> +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb.c
>
>> +int msm_wb_modeset_init(struct msm_wb *wb,
>> + struct drm_device *dev, struct drm_encoder *encoder)
>> +{
>> + struct msm_drm_private *priv = dev->dev_private;
>> + int ret;
>> +
>> + wb->dev = dev;
>> + wb->encoder = encoder;
>> +
>> + wb->connector = msm_wb_connector_init(wb);
>> + if (IS_ERR(wb->connector)) {
>> + ret = PTR_ERR(wb->connector);
>> + dev_err(dev->dev, "failed to create WB connector: %d\n",
>> ret);
>> + wb->connector = NULL;
>> + goto fail;
>> + }
>> +
>> + priv->connectors[priv->num_connectors++] = wb->connector;
>> +
>> + return 0;
>> +
>> +fail:
>> + if (wb->connector) {
>> + wb->connector->funcs->destroy(wb->connector);
>> + wb->connector = NULL;
>> + }
>> +
> Drop the unused if block ?
>
>
>> +static struct msm_wb *msm_wb_init(struct platform_device *pdev)
>> +{
>> + struct msm_wb *wb = NULL;
>> +
>> + wb = devm_kzalloc(&pdev->dev, sizeof(*wb), GFP_KERNEL);
>> + if (!wb)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + wb->pdev = pdev;
>> + wb->priv_data = devm_kzalloc(&pdev->dev, sizeof(*wb->priv_data),
>> + GFP_KERNEL);
>> + if (!wb->priv_data)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + if (msm_wb_v4l2_init(wb)) {
>> + pr_err("%s: wb v4l2 init failed\n", __func__);
>> + return ERR_PTR(-ENODEV);
>> + }
>> +
> Seems like we're leaking wb and/or wb->priv_data. Add a label and
> consolidate error handling in there ?
Since the devm_kzalloc function is used here, the system should take care
freeing the memory.
>
>
>> +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb.h
>
>> +#ifndef __WB_CONNECTOR_H__
>> +#define __WB_CONNECTOR_H__
>> +
> The file is called mdp_wb.h, so one might want to change this to
> __MDP_WB_H__
>
>
>> --- /dev/null
>> +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb_v4l2.c
>> @@ -0,0 +1,522 @@
>
>> +static const struct msm_wb_fmt *get_format(u32 fourcc)
>> +{
>> + const struct msm_wb_fmt *fmt;
>> + unsigned int k;
>> +
>> + for (k = 0; k < ARRAY_SIZE(formats); k++) {
>> + fmt = &formats[k];
>> + if (fmt->fourcc == fourcc)
>> + break;
>> + }
>> +
>> + if (k == ARRAY_SIZE(formats))
>> + return NULL;
>> +
>> + return &formats[k];
> You could move the return within the loop, and drop the follow up
> conditional.
>
>
>> +static void *msm_wb_vb2_attach_dmabuf(void *alloc_ctx, struct dma_buf
>> *dbuf,
>> + unsigned long size, int write)
>> +{
>> + struct msm_wb_v4l2_dev *dev = alloc_ctx;
>> + struct drm_device *drm_dev = dev->wb->dev;
>> + struct drm_gem_object *obj;
>> +
>> + mutex_lock(&drm_dev->object_name_lock);
>> + obj = drm_dev->driver->gem_prime_import(drm_dev, dbuf);
>> + if (WARN_ON(!obj)) {
>> + mutex_unlock(&drm_dev->object_name_lock);
>> + v4l2_err(&dev->v4l2_dev, "Can't convert dmabuf to gem
>> obj.\n");
>> + return NULL;
> Shouldn't one return ERR_PTR here ? Consolidating the error paths to a
> single label will be cleaner imho.
>
>
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/driver
[PATCH 2/3] drm:msm: Initial Add Writeback Support
> On Thu, Apr 2, 2015 at 2:24 PM, Paul Bolle wrote:
>> Hi Jilai,
>>
>> On Thu, 2015-04-02 at 17:58 +, jilaiw at codeaurora.org wrote:
>>> Thanks Paul. Some comments embedded and for the rest I will update the
>>> code accordingly.
>>
>> Most of my comments appear to be ill informed, so I wouldn't mind if
>> you'd specify which updates you actually plan to do.
>>
>>> >> --- a/drivers/gpu/drm/msm/Makefile
>>> >> +++ b/drivers/gpu/drm/msm/Makefile
>>> >
>>> >> +msm-$(CONFIG_DRM_MSM_WB) += \
>>> >> + mdp/mdp5/mdp5_wb_encoder.o \
>>> >> + mdp/mdp_wb/mdp_wb.o \
>>> >> + mdp/mdp_wb/mdp_wb_connector.o \
>>> >> + mdp/mdp_wb/mdp_wb_v4l2.o
>>> >
>>> > so mdp_wb_v4l2.o will never be part of a module.
>>> If CONFIG-DRM_MSM_WB is y, then all wb related files (including
>>> mdp_wb_v4l2.o) will be added to msm-y, then be linked to msm.o
>>> obj-$(CONFIG_DRM_MSM) += msm.o
>>
>> (A tell tale was that I didn't quote that line.)
>>
>>> Refer to document http://lwn.net/Articles/21835/ (section 3.3),
>>> it should be able to be built-in to kernel or as a module.
>>
>> It's hard typing with a brown paper bag for headware: I still have
>> trouble speaking Makefile, even after all these years, but I'm afraid
>> you're right.
>>
>>> >> --- /dev/null
>>> >> +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb_v4l2.c
>>> >
>>> >> +#include
>>> >
>>> > This include is needed mostly for module_param(), right?
>>> >
>>> >> +#define MSM_WB_MODULE_NAME "msm_wb"
>>> >
>>> > MSM_WB_DRIVER_NAME? But see below.
>>> >
>>> >> +static unsigned debug;
>>> >> +module_param(debug, uint, 0644);
>>> >
>>> > debug is basically a boolean type of flag. Would
>>> > static bool debug;
>>> > module_param(debug, bool, 0644);
>>> >
>>> > work?
>>> >
>>> >> +MODULE_PARM_DESC(debug, "activates debug info");
>>> >
>>> > No one is ever going to see this description.
>>> >
>>> >> +#define dprintk(dev, level, fmt, arg...) \
>>> >> + v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
>>> >
>>> > All instances of dprintk() that I found had level set to 1, so the
>>> above
>>> > could be simplified a bit:
>>> > #define dprintk(dev, fmt, arg...) \
>>> > v4l2_dbg(1, debug, &dev->v4l2_dev, fmt, ## arg)
>>> >
>>> > But perhaps pr_debug() and the dynamic debug facility could be used
>>> > instead of module_param(), dprintk() and v4l2_dbg(). Not sure which
>>> > approach is easier.
>>> >
>>> >> +static const struct v4l2_file_operations msm_wb_v4l2_fops = {
>>> >> + .owner = THIS_MODULE,
>>> >
>>> > THIS_MODULE will basically be equivalent to NULL.
>>> >
>>> >> + .open = v4l2_fh_open,
>>> >> + .release = vb2_fop_release,
>>> >> + .poll = vb2_fop_poll,
>>> >> + .unlocked_ioctl = video_ioctl2,
>>> >> +};
>>> >
>>> >> +int msm_wb_v4l2_init(struct msm_wb *wb)
>>> >> +{
>>> >> + struct msm_wb_v4l2_dev *dev;
>>> >> + struct video_device *vfd;
>>> >> + struct vb2_queue *q;
>>> >> + int ret;
>>> >> +
>>> >> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>>> >> + if (!dev)
>>> >> + return -ENOMEM;
>>> >> +
>>> >> + snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name),
>>> >> + "%s", MSM_WB_MODULE_NAME);
>>> >
>>> > It looks like you can actually drop the #define and merge the last
>>> two
>>> > arguments to snprintf() into just "msm_wb".
>>> >
>>> >> + ret = v4l2_device_register(NULL, &dev->v4l2_dev);
>>> >> + if (ret)
>>> >> + goto free_dev;
>>> >> +
>>> >> + /* default ARGB 640x480 */
>>> >> + dev->fmt = get_format(V4L2_PIX_FMT_RGB32);
>>> >> + dev->width = 640;
>>> >> + dev->height = 480;
>>> >> +
>>> >> + /* initialize queue */
>>> >> + q = &dev->vb_vidq;
>>> >> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
>>> >> + q->io_modes = VB2_DMABUF;
>>> >> + q->drv_priv = dev;
>>> >> + q->buf_struct_size = sizeof(struct msm_wb_v4l2_buffer);
>>> >> + q->ops = &msm_wb_vb2_ops;
>>> >> + q->mem_ops = &msm_wb_vb2_mem_ops;
>>> >> + q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>>> >> +
>>> >> + ret = vb2_queue_init(q);
>>> >> + if (ret)
>>> >> + goto unreg_dev;
>>> >> +
>>> >> + mutex_init(&dev->mutex);
>>> >> +
>>> >> + vfd = &dev->vdev;
>>> >> + *vfd = msm_wb_v4l2_template;
>>> >> + vfd->debug = debug;
>>> >
>>> > I couldn't find a member of struct video_device named debug. Where
>>> does
>>> > that come from? Because, as far as I can see, this won't compile.
>>> yes, it's there:
>>> http://lxr.free-electrons.com/source/include/media/v4l2-dev.h#L127
>>
>> Probably in some tree I'm not aware of. I only did:
>> $ git cat-file blob v4.0-rc6:include/media/v4l2-dev.h | grep debug
>> /* Internal device debug flags, not for use by drivers */
>> int dev_debug;
>>
>> and
>> $ git cat-file blob next-20150402:include/media/v4l2-dev.h | grep
>> debug
>> /* Internal device debug flags, not for use by drivers */
>> int dev_debug;
>>
>> Which tree does debug come from?
>
> fwiw, looks like 17028cd renamed debug -> dev_debug
>
> BR,
> -R
>
Thanks, R
[PATCH 2/3] drm:msm: Initial Add Writeback Support
> On Thu, Apr 2, 2015 at 2:24 PM, Paul Bolle wrote:
>> Hi Jilai,
>>
>> On Thu, 2015-04-02 at 17:58 +, jilaiw at codeaurora.org wrote:
>>> Thanks Paul. Some comments embedded and for the rest I will update the
>>> code accordingly.
>>
>> Most of my comments appear to be ill informed, so I wouldn't mind if
>> you'd specify which updates you actually plan to do.
>>
>>> >> --- a/drivers/gpu/drm/msm/Makefile
>>> >> +++ b/drivers/gpu/drm/msm/Makefile
>>> >
>>> >> +msm-$(CONFIG_DRM_MSM_WB) += \
>>> >> + mdp/mdp5/mdp5_wb_encoder.o \
>>> >> + mdp/mdp_wb/mdp_wb.o \
>>> >> + mdp/mdp_wb/mdp_wb_connector.o \
>>> >> + mdp/mdp_wb/mdp_wb_v4l2.o
>>> >
>>> > so mdp_wb_v4l2.o will never be part of a module.
>>> If CONFIG-DRM_MSM_WB is y, then all wb related files (including
>>> mdp_wb_v4l2.o) will be added to msm-y, then be linked to msm.o
>>> obj-$(CONFIG_DRM_MSM) += msm.o
>>
>> (A tell tale was that I didn't quote that line.)
>>
>>> Refer to document http://lwn.net/Articles/21835/ (section 3.3),
>>> it should be able to be built-in to kernel or as a module.
>>
>> It's hard typing with a brown paper bag for headware: I still have
>> trouble speaking Makefile, even after all these years, but I'm afraid
>> you're right.
>>
>>> >> --- /dev/null
>>> >> +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb_v4l2.c
>>> >
>>> >> +#include
>>> >
>>> > This include is needed mostly for module_param(), right?
>>> >
>>> >> +#define MSM_WB_MODULE_NAME "msm_wb"
>>> >
>>> > MSM_WB_DRIVER_NAME? But see below.
>>> >
>>> >> +static unsigned debug;
>>> >> +module_param(debug, uint, 0644);
>>> >
>>> > debug is basically a boolean type of flag. Would
>>> > static bool debug;
>>> > module_param(debug, bool, 0644);
>>> >
>>> > work?
>>> >
>>> >> +MODULE_PARM_DESC(debug, "activates debug info");
>>> >
>>> > No one is ever going to see this description.
>>> >
>>> >> +#define dprintk(dev, level, fmt, arg...) \
>>> >> + v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg)
>>> >
>>> > All instances of dprintk() that I found had level set to 1, so the
>>> above
>>> > could be simplified a bit:
>>> > #define dprintk(dev, fmt, arg...) \
>>> > v4l2_dbg(1, debug, &dev->v4l2_dev, fmt, ## arg)
>>> >
>>> > But perhaps pr_debug() and the dynamic debug facility could be used
>>> > instead of module_param(), dprintk() and v4l2_dbg(). Not sure which
>>> > approach is easier.
>>> >
>>> >> +static const struct v4l2_file_operations msm_wb_v4l2_fops = {
>>> >> + .owner = THIS_MODULE,
>>> >
>>> > THIS_MODULE will basically be equivalent to NULL.
>>> >
>>> >> + .open = v4l2_fh_open,
>>> >> + .release = vb2_fop_release,
>>> >> + .poll = vb2_fop_poll,
>>> >> + .unlocked_ioctl = video_ioctl2,
>>> >> +};
>>> >
>>> >> +int msm_wb_v4l2_init(struct msm_wb *wb)
>>> >> +{
>>> >> + struct msm_wb_v4l2_dev *dev;
>>> >> + struct video_device *vfd;
>>> >> + struct vb2_queue *q;
>>> >> + int ret;
>>> >> +
>>> >> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>>> >> + if (!dev)
>>> >> + return -ENOMEM;
>>> >> +
>>> >> + snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name),
>>> >> + "%s", MSM_WB_MODULE_NAME);
>>> >
>>> > It looks like you can actually drop the #define and merge the last
>>> two
>>> > arguments to snprintf() into just "msm_wb".
>>> >
>>> >> + ret = v4l2_device_register(NULL, &dev->v4l2_dev);
>>> >> + if (ret)
>>> >> + goto free_dev;
>>> >> +
>>> >> + /* default ARGB 640x480 */
>>> >> + dev->fmt = get_format(V4L2_PIX_FMT_RGB32);
>>> >> + dev->width = 640;
>>> >> + dev->height = 480;
>>> >> +
>>> >> + /* initialize queue */
>>> >> + q = &dev->vb_vidq;
>>> >> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
>>> >> + q->io_modes = VB2_DMABUF;
>>> >> + q->drv_priv = dev;
>>> >> + q->buf_struct_size = sizeof(struct msm_wb_v4l2_buffer);
>>> >> + q->ops = &msm_wb_vb2_ops;
>>> >> + q->mem_ops = &msm_wb_vb2_mem_ops;
>>> >> + q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>>> >> +
>>> >> + ret = vb2_queue_init(q);
>>> >> + if (ret)
>>> >> + goto unreg_dev;
>>> >> +
>>> >> + mutex_init(&dev->mutex);
>>> >> +
>>> >> + vfd = &dev->vdev;
>>> >> + *vfd = msm_wb_v4l2_template;
>>> >> + vfd->debug = debug;
>>> >
>>> > I couldn't find a member of struct video_device named debug. Where
>>> does
>>> > that come from? Because, as far as I can see, this won't compile.
>>> yes, it's there:
>>> http://lxr.free-electrons.com/source/include/media/v4l2-dev.h#L127
>>
>> Probably in some tree I'm not aware of. I only did:
>> $ git cat-file blob v4.0-rc6:include/media/v4l2-dev.h | grep debug
>> /* Internal device debug flags, not for use by drivers */
>> int dev_debug;
>>
>> and
>> $ git cat-file blob next-20150402:include/media/v4l2-dev.h | grep
>> debug
>> /* Internal device debug flags, not for use by drivers */
>> int dev_debug;
>>
>> Which tree does debug come from?
>
> fwiw, looks like 17028cd renamed debug -> dev_debug
>
> BR,
> -R
>
Thanks, R
[PATCH 2/3] drm:msm: Initial Add Writeback Support
> On Thu, Apr 02, 2015 at 10:29:52AM -0400, Rob Clark wrote: >> So, from a quick look, it seems like there is a lot of potential to >> split the v4l part out into some drm helpers.. it looks pretty >> generic(ish), or at least it could be with some strategically placed >> vfuncs in drm_v4l2_helper_funcs. >> >> I do think we need to figure out the auth/security situation. We >> probably don't want to let arbitrary processes open a v4l device and >> snoop on the screen contents. We perhaps could re-use the dri2 drm >> auth stuff (v4l2_drm_get_magic ioctl?). Or, well, it would be nice if >> the wb device could be made to not exist in /dev at all, and >> pre-open'd fd returned from an ioctl on the drm device, but not really >> sure if that is possible (or too weird). Once the compositor process >> has the v4l device open and authenticated somehow, I expect it would >> use fd passing to pass the fd off to a trusted helper process. > > Please don't resurrect the magic stuff ;-) > > Anyway I discussed this a bit with Laurent and we figured the best way to > wire up writeback support is by using drm framebuffers. Then you can use > atomic flips to create a new snapshot. Of course that won't work with hw > where writeback is continuous, there v4l is a much better fit. And we also > have hardware where some v4l pipeline could directly feed into a drm > output pipeline, so we need a generic way to connect v4l and drm anyway. > For that I think we should add a new flag to addfb2 (or a new addfbv4l) > which creates a magic framebuffer from a v4l input/output. Some values > like stride don't make sense in such a virtual framebuffer, but pixel > format and size are all needed. > > This way we don't need parallel abis for single-shot writeback directly > into framebuffers and for continuous writeback through v4l, we can reuse > the same drm framebuffer ones. And this also solves the security issues > since no one can start writeback without the drm device owner's consent, > so no need to reinvent anything there. And with atomic we already have > almost everything there: For the writeback framebuffer we only need a new > "WRITEBACK" property (which takes an fb id) and the small extension to > create v4l-backed framebuffers. > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > Hi Daniel, 1. This change is to implement a continuous writeback. 2. As you said, we need "a generic way to connect v4l and drm". Especially how to share the buffer information between v4l and drm for writeback output. Below are just some details of this change: In current implementation, I expect the output buffer is dma buffer which could be from GEM object (drm) or from video encoder (V4l). Once the buffer is queued into V4l driver, it will be converted into a GEM object and then pass it to drm as writeback output buffer. Once the buffer is captured, it will notify V4l driver to let user dequeue buffer. Drm will notice there is a Virtual Connector (maybe a new type WRITEBACK can be added), but it will only be "connected" until V4l starts streaming. During streaming, drm application just does normal page_flip while v4l application queues/dequeues v4l buffers. Thanks, Jilai
[PATCH 2/3] drm:msm: Initial Add Writeback Support
> On Tue, Apr 07, 2015 at 03:55:45PM -, jilaiw at codeaurora.org wrote: >> > On Thu, Apr 02, 2015 at 10:29:52AM -0400, Rob Clark wrote: >> >> So, from a quick look, it seems like there is a lot of potential to >> >> split the v4l part out into some drm helpers.. it looks pretty >> >> generic(ish), or at least it could be with some strategically placed >> >> vfuncs in drm_v4l2_helper_funcs. >> >> >> >> I do think we need to figure out the auth/security situation. We >> >> probably don't want to let arbitrary processes open a v4l device and >> >> snoop on the screen contents. We perhaps could re-use the dri2 drm >> >> auth stuff (v4l2_drm_get_magic ioctl?). Or, well, it would be nice >> if >> >> the wb device could be made to not exist in /dev at all, and >> >> pre-open'd fd returned from an ioctl on the drm device, but not >> really >> >> sure if that is possible (or too weird). Once the compositor process >> >> has the v4l device open and authenticated somehow, I expect it would >> >> use fd passing to pass the fd off to a trusted helper process. >> > >> > Please don't resurrect the magic stuff ;-) >> > >> > Anyway I discussed this a bit with Laurent and we figured the best way >> to >> > wire up writeback support is by using drm framebuffers. Then you can >> use >> > atomic flips to create a new snapshot. Of course that won't work with >> hw >> > where writeback is continuous, there v4l is a much better fit. And we >> also >> > have hardware where some v4l pipeline could directly feed into a drm >> > output pipeline, so we need a generic way to connect v4l and drm >> anyway. >> > For that I think we should add a new flag to addfb2 (or a new >> addfbv4l) >> > which creates a magic framebuffer from a v4l input/output. Some values >> > like stride don't make sense in such a virtual framebuffer, but pixel >> > format and size are all needed. >> > >> > This way we don't need parallel abis for single-shot writeback >> directly >> > into framebuffers and for continuous writeback through v4l, we can >> reuse >> > the same drm framebuffer ones. And this also solves the security >> issues >> > since no one can start writeback without the drm device owner's >> consent, >> > so no need to reinvent anything there. And with atomic we already have >> > almost everything there: For the writeback framebuffer we only need a >> new >> > "WRITEBACK" property (which takes an fb id) and the small extension to >> > create v4l-backed framebuffers. >> > >> > Cheers, Daniel >> > -- >> > Daniel Vetter >> > Software Engineer, Intel Corporation >> > http://blog.ffwll.ch >> > >> Hi Daniel, >> >> 1. This change is to implement a continuous writeback. >> 2. As you said, we need "a generic way to connect v4l and drm". >> Especially how to share the buffer information between v4l and drm for >> writeback output. >> >> Below are just some details of this change: >> >> In current implementation, I expect the output buffer is dma buffer >> which could be from GEM object (drm) or from video encoder (V4l). Once >> the buffer is queued into V4l driver, it will be converted into a GEM >> object and then pass it to drm as writeback output buffer. Once the >> buffer is captured, it will notify V4l driver to let user dequeue >> buffer. >> >> Drm will notice there is a Virtual Connector (maybe a new type WRITEBACK >> can be added), but it will only be "connected" until V4l >> starts streaming. > > Yes we definitely should add a new connector type WRITEBACK. And just the > connector kinda works for your hw design where writeback works like a > separate encoder. But there's also hw out there where any crtc can be > written back, and for those cases we need explicit properties. Then > there's also the one-shot vs. continuous issues. yes, this change is specific for msm chip. In order to cover the other writeback use cases, new properties and framework changes are required. > > Given all that I still think you want an explicit drm framebuffer to > connect the kms and the v4l side of things. That would also help a bit > with making it clear which v4l connects to which drm device. yes, it will help. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch >
